Re: [PATCH] qmp: Stabilize preconfig
On 11/11/21 07:11, Markus Armbruster wrote: Paolo Bonzini writes: On 11/3/21 09:02, Markus Armbruster wrote: I wonder whether we really have to step through three states x-exit-preconfig cont preconfig ---> pre run ---> run and not two cont pre run ---> run Devices would be hotplugged between x-exit-preconfig and cont, and Cold plugged! Nope, hotplugged! After x-exit-preconfig, the machine is ready to start, and that means that the machine will have completed initialization via their machine_init_done notifiers. For example, fw_cfg will have set the bootorder. Any device created after x-exit-preconfig will not be in the bootorder. part of the machine until x-exit-preconfig; so there is a need for something like x-exit-preconfig. Can you briefly explain why device_add doesn't work before x-exit-preconfig and does after? The answer to this question is basically the verbose version of the coldplug/hotplug thing from above. There are five stages in the startup of QEMU (marked by different values of the MachineInitPhase enum): 1) PHASE_NO_MACHINE - backends can already be created here, but no machine exists yet 2) PHASE_MACHINE_CREATED - the machine object has been created. It's not initialized, but it's there. 3) PHASE_ACCEL_CREATED - the accelerator object has been created. The accelerator needs the machine object, because for example KVM might not support all machine types. So the accelerator queries the machine object and fails creation in case of incompatibility. This enables e.g. fallback to TCG. -preconfig starts the monitor here. 4) PHASE_MACHINE_INIT - machine initialization consists mostly in creating the onboard devices. For this to happen, the machine needs to learn about the accelerator, because onboard devices include CPUs and other accelerator-dependent devices. Devices plugged in this phase are cold-plugged. 5) PHASE_MACHINE_READY - machine init done notifiers have been called and the VM is ready. Devices plugged in this phase already count as hot-plugged. -S starts the monitor here. x-exit-preconfig goes straight from PHASE_ACCEL_CREATED to PHASE_MACHINE_READY. Devices can only be created after PHASE_MACHINE_INIT, so device_add cannot be enabled at preconfig stage. Why does preconfig start at PHASE_ACCEL_CREATED? Well, the phases were not as easy to identify in qemu_init() when it was introduced, so I suppose it just seemed like a good place. :) These days, qemu_init() is just a hundred lines of code apart from the huge command line parsing switch statement, so we have a clearer idea of the steps and you can look deeper at what happens in each phase if you want. phase_advance() is your friend. With a pure-QMP configuration flow, PHASE_MACHINE_CREATED would be reached with a machine-set command (corresponding to the non-deprecated parts of -machine) and PHASE_ACCEL_CREATED would be reached with an accel-set command (corresponding to -accel). I haven't yet thought hard enough whether accel-set could go directly from PHASE_MACHINE_CREATED to PHASE_MACHINE_INIT. It probably depends on how CPUs would be configured in the QMP flow; if accel-set must return at PHASE_ACCEL_CREATED, a separate command is needed to reach PHASE_MACHINE_INIT. But either way, there the monitor would be accessible at PHASE_MACHINE_INIT, where device_add works and cold-plugs the devices. Paolo
Re: [PATCH v2 1/2] tests/unit/test-smp-parse: Make an unified name for the tested machine
On Thu, Nov 11, 2021 at 10:44:28AM +0800, Yanan Wang wrote:
> Currently, the name of the tested machine in the expected error
> messages is hardcoded as "(null)" which is not good, because the
> actual generated name of the machine maybe "(null)" or "(NULL)"
> which will cause an unexpected test failure in some CI platforms.
>
> So let's rename the tested machine with an unified string and
> tweak the expected error messages accordingly.
>
> Fixes: 9e8e393bb7 ("tests/unit: Add an unit test for smp parsing")
> Reported-by: Philippe Mathieu-Daudé
> Signed-off-by: Yanan Wang
> ---
> tests/unit/test-smp-parse.c | 38 ++---
> 1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index cbe0c99049..872512aa37 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -23,6 +23,8 @@
> #define MIN_CPUS 1 /* set the min CPUs supported by the machine as 1 */
> #define MAX_CPUS 512 /* set the max CPUs supported by the machine as 512 */
>
> +#define SMP_MACHINE_NAME "TEST-SMP"
> +
> /*
> * Used to define the generic 3-level CPU topology hierarchy
> * -sockets/cores/threads
> @@ -315,13 +317,13 @@ static struct SMPTestData data_generic_invalid[] = {
> * should tweak the supported min CPUs to 2 for testing */
> .config = SMP_CONFIG_GENERIC(T, 1, F, 0, F, 0, F, 0, F, 0),
> .expect_error = "Invalid SMP CPUs 1. The min CPUs supported "
> -"by machine '(null)' is 2",
> +"by machine '" SMP_MACHINE_NAME "' is 2",
> }, {
> /* config: -smp 512
> * should tweak the supported max CPUs to 511 for testing */
> .config = SMP_CONFIG_GENERIC(T, 512, F, 0, F, 0, F, 0, F, 0),
> .expect_error = "Invalid SMP CPUs 512. The max CPUs supported "
> -"by machine '(null)' is 511",
> +"by machine '" SMP_MACHINE_NAME "' is 511",
> },
> };
>
> @@ -480,26 +482,41 @@ static void unsupported_params_init(MachineClass *mc,
> SMPTestData *data)
> }
> }
>
> -/* Reset the related machine properties before each sub-test */
> -static void smp_machine_class_init(MachineClass *mc)
> +static Object *smp_test_machine_init(void)
> {
> +Object *obj = object_new(TYPE_MACHINE);
> +MachineClass *mc = MACHINE_GET_CLASS(obj);
> +
> +g_free(mc->name);
> +mc->name = g_strdup(SMP_MACHINE_NAME);
> +
> mc->min_cpus = MIN_CPUS;
> mc->max_cpus = MAX_CPUS;
>
> mc->smp_props.prefer_sockets = true;
> mc->smp_props.dies_supported = false;
> +
> +return obj;
> +}
> +
> +static void smp_test_machine_deinit(Object *obj)
> +{
> +MachineClass *mc = MACHINE_GET_CLASS(obj);
> +
> +g_free(mc->name);
> +mc->name = NULL;
> +
> +object_unref(obj);
> }
>
> static void test_generic(void)
> {
> -Object *obj = object_new(TYPE_MACHINE);
> +Object *obj = smp_test_machine_init();
> MachineState *ms = MACHINE(obj);
> MachineClass *mc = MACHINE_GET_CLASS(obj);
> SMPTestData *data = &(SMPTestData){{ }};
> int i;
>
> -smp_machine_class_init(mc);
> -
> for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> *data = data_generic_valid[i];
> unsupported_params_init(mc, data);
> @@ -523,19 +540,18 @@ static void test_generic(void)
> smp_parse_test(ms, data, false);
> }
>
> -object_unref(obj);
> +smp_test_machine_deinit(obj);
> }
>
> static void test_with_dies(void)
> {
> -Object *obj = object_new(TYPE_MACHINE);
> +Object *obj = smp_test_machine_init();
> MachineState *ms = MACHINE(obj);
> MachineClass *mc = MACHINE_GET_CLASS(obj);
> SMPTestData *data = &(SMPTestData){{ }};
> unsigned int num_dies = 2;
> int i;
>
> -smp_machine_class_init(mc);
> mc->smp_props.dies_supported = true;
>
> for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> @@ -575,7 +591,7 @@ static void test_with_dies(void)
> smp_parse_test(ms, data, false);
> }
>
> -object_unref(obj);
> +smp_test_machine_deinit(obj);
> }
>
> int main(int argc, char *argv[])
> --
> 2.19.1
>
Reviewed-by: Andrew Jones
Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports
On Thu, Nov 11, 2021 at 08:53:06AM +0100, Gerd Hoffmann wrote: > Hi, > > > Given it's a bugfix, and given that I hear through internal channels > > that QE results so far have been encouraging, I am inclined to bite the > > bullet and merge this for -rc1. > > Fine with me. > > > I don't think this conflicts with Julia's patches as users can still > > disable ACPI hotplug into bridges. Gerd, agree? Worth the risk? > > Combining this with Julia's patches looks a bit risky to me and surely > needs testing. I expect the problematic case is both native and acpi > hotplug being enabled. > When the guest uses acpi hotplug nobody will > turn on slot power on the pcie root port ... I'm not sure I understand what the situation is, and how to trigger it. Could you clarify pls? > I'd suggest to just revert to pcie native hotplug for 6.2. Hmm that kind of change seems even riskier to me. I think I'll try with Igor's patches. > Give acpi > hotplug another try for 7.0, and post patches early in the devel cycle > then instead of waiting until -rc0 is released. > > take care, > Gerd
Re: [PATCH v3 3/5] numa: Support SGX numa in the monitor and Libvirt interfaces
On Thu, Nov 11, 2021 at 08:55:35AM +0100, Philippe Mathieu-Daudé wrote:
> On 11/11/21 07:18, Yang Zhong wrote:
> > On Wed, Nov 10, 2021 at 10:55:40AM -0600, Eric Blake wrote:
> >> On Mon, Nov 01, 2021 at 12:20:07PM -0400, Yang Zhong wrote:
> >>> Add the SGXEPCSection list into SGXInfo to show the multiple
> >>> SGX EPC sections detailed info, not the total size like before.
> >>> This patch can enable numa support for 'info sgx' command and
> >>> QMP interfaces. The new interfaces show each EPC section info
> >>> in one numa node. Libvirt can use QMP interface to get the
> >>> detailed host SGX EPC capabilities to decide how to allocate
> >>> host EPC sections to guest.
> >>>
> >>> (qemu) info sgx
> >>> SGX support: enabled
> >>> SGX1 support: enabled
> >>> SGX2 support: enabled
> >>> FLC support: enabled
> >>> NUMA node #0: size=67108864
> >>> NUMA node #1: size=29360128
> >>>
> >>> The QMP interface show:
> >>> (QEMU) query-sgx
> >>> {"return": {"sgx": true, "sgx2": true, "sgx1": true, "sections": \
> >>> [{"node": 0, "size": 67108864}, {"node": 1, "size": 29360128}], "flc":
> >>> true}}
> >>>
> >>> (QEMU) query-sgx-capabilities
> >>> {"return": {"sgx": true, "sgx2": true, "sgx1": true, "sections": \
> >>> [{"node": 0, "size": 17070817280}, {"node": 1, "size": 17079205888}],
> >>> "flc": true}}
> >>
> >> Other than the different "size" values, how do these commands differ?
> >
> >
> > As for QMP interfaces,
> > The 'query-sgx' to get VM sgx detailed info, and 'query-sgx-capabilities'
> > to get
> > the host sgx capabilities and Libvirt can use this info to decide how to
> > allocate
> > virtual EPC sections to VMs.
>
> What about renaming/aliasing as 'query-host-sgx' / 'query-guest-sgx'?
The current Libvirt and Qemu's QMP interface define all interfaces like those
naming
rule. If we change those names, there are lots of work in the Qemu and
Libvirt sides.
Thanks!
Yang
Re: [PATCH 1/2] virtio: use virtio accessor to access packed descriptor flags
On Thu, Nov 11, 2021 at 02:38:53PM +0800, Jason Wang wrote:
> We used to access packed descriptor flags via
> address_space_{write|read}_cached(). When we hit the cache, memcpy()
> is used which is not an atomic operation which may lead a wrong value
> is read or wrote.
Could you clarify where's the memcpy that you see?
Thanks!
> So this patch switches to use virito_{stw|lduw}_phys_cached() to make
> sure the aceess is atomic.
>
> Fixes: 86044b24e865f ("virtio: basic packed virtqueue support")
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Jason Wang
> ---
> hw/virtio/virtio.c | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index cc69a9b881..939bcbfeb9 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -507,11 +507,9 @@ static void vring_packed_desc_read_flags(VirtIODevice
> *vdev,
> MemoryRegionCache *cache,
> int i)
> {
> -address_space_read_cached(cache,
> - i * sizeof(VRingPackedDesc) +
> - offsetof(VRingPackedDesc, flags),
> - flags, sizeof(*flags));
> -virtio_tswap16s(vdev, flags);
> +hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc,
> flags);
> +
> +*flags = virtio_lduw_phys_cached(vdev, cache, off);
> }
>
> static void vring_packed_desc_read(VirtIODevice *vdev,
> @@ -564,8 +562,7 @@ static void vring_packed_desc_write_flags(VirtIODevice
> *vdev,
> {
> hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc,
> flags);
>
> -virtio_tswap16s(vdev, &desc->flags);
> -address_space_write_cached(cache, off, &desc->flags,
> sizeof(desc->flags));
> +virtio_stw_phys_cached(vdev, cache, off, desc->flags);
> address_space_cache_invalidate(cache, off, sizeof(desc->flags));
> }
>
> --
> 2.25.1
Re: [PATCH for 6.2 v2 5/5] bios-tables-test: Update golden binaries
On Wed, Nov 10, 2021 at 04:11:40PM -0500, Igor Mammedov wrote:
> From: Julia Suvorova
>
> The changes are the result of
> 'hw/i386/acpi-build: Deny control on PCIe Native Hot-Plug in _OSC'
> and listed here:
>
> Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities
> {
> CreateDWordField (Arg3, Zero, CDW1)
> If ((Arg0 == ToUUID ("33db4d5b-1ff7-401c-9657-7441c03dd766")
> /* PCI Host Bridge Device */))
> {
> CreateDWordField (Arg3, 0x04, CDW2)
> CreateDWordField (Arg3, 0x08, CDW3)
> Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> -Local0 &= 0x1F
> +Local0 &= 0x1E
>
> Signed-off-by: Julia Suvorova
> Signed-off-by: Igor Mammedov
> ---
> tests/qtest/bios-tables-test-allowed-diff.h | 16
> tests/data/acpi/q35/DSDT| Bin 8289 -> 8289 bytes
> tests/data/acpi/q35/DSDT.acpihmat | Bin 9614 -> 9614 bytes
> tests/data/acpi/q35/DSDT.bridge | Bin 11003 -> 11003 bytes
> tests/data/acpi/q35/DSDT.cphp | Bin 8753 -> 8753 bytes
> tests/data/acpi/q35/DSDT.dimmpxm| Bin 9943 -> 9943 bytes
> tests/data/acpi/q35/DSDT.dmar | Bin 0 -> 8289 bytes
> tests/data/acpi/q35/DSDT.ipmibt | Bin 8364 -> 8364 bytes
> tests/data/acpi/q35/DSDT.ivrs | Bin 8306 -> 8306 bytes
> tests/data/acpi/q35/DSDT.memhp | Bin 9648 -> 9648 bytes
> tests/data/acpi/q35/DSDT.mmio64 | Bin 9419 -> 9419 bytes
> tests/data/acpi/q35/DSDT.multi-bridge | Bin 8583 -> 8583 bytes
> tests/data/acpi/q35/DSDT.nohpet | Bin 8147 -> 8147 bytes
> tests/data/acpi/q35/DSDT.nosmm | Bin 0 -> 8289 bytes
> tests/data/acpi/q35/DSDT.numamem| Bin 8295 -> 8295 bytes
> tests/data/acpi/q35/DSDT.smm-compat | Bin 0 -> 8289 bytes
> tests/data/acpi/q35/DSDT.smm-compat-nosmm | Bin 0 -> 8289 bytes
> tests/data/acpi/q35/DSDT.tis.tpm12 | Bin 8894 -> 8894 bytes
> tests/data/acpi/q35/DSDT.tis.tpm2 | Bin 8894 -> 8894 bytes
> tests/data/acpi/q35/DSDT.xapic | Bin 35652 -> 35652 bytes
Why do we have all the new files? What is going on here?
> 20 files changed, 16 deletions(-)
> create mode 100644 tests/data/acpi/q35/DSDT.dmar
> create mode 100644 tests/data/acpi/q35/DSDT.nosmm
> create mode 100644 tests/data/acpi/q35/DSDT.smm-compat
> create mode 100644 tests/data/acpi/q35/DSDT.smm-compat-nosmm
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h
> b/tests/qtest/bios-tables-test-allowed-diff.h
> index 48e5634d4b..dfb8523c8b 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,17 +1 @@
> /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/q35/DSDT",
> -"tests/data/acpi/q35/DSDT.tis",
> -"tests/data/acpi/q35/DSDT.bridge",
> -"tests/data/acpi/q35/DSDT.mmio64",
> -"tests/data/acpi/q35/DSDT.ipmibt",
> -"tests/data/acpi/q35/DSDT.cphp",
> -"tests/data/acpi/q35/DSDT.memhp",
> -"tests/data/acpi/q35/DSDT.acpihmat",
> -"tests/data/acpi/q35/DSDT.numamem",
> -"tests/data/acpi/q35/DSDT.dimmpxm",
> -"tests/data/acpi/q35/DSDT.nohpet",
> -"tests/data/acpi/q35/DSDT.tis.tpm2",
> -"tests/data/acpi/q35/DSDT.tis.tpm12",
> -"tests/data/acpi/q35/DSDT.multi-bridge",
> -"tests/data/acpi/q35/DSDT.ivrs",
> -"tests/data/acpi/q35/DSDT.xapic",
> diff --git a/tests/data/acpi/q35/DSDT b/tests/data/acpi/q35/DSDT
> index
> 281fc82c03b2562d2e6b7caec0d817b034a47138..c1965f6051ef2af81dd8412abe169d87845bb033
> 100644
> GIT binary patch
> delta 24
> gcmaFp@X&$FCDET<0BnZ{w*UYD
>
> delta 24
> gcmaFp@X&$FCDET<0BnK?w*UYD
>
> diff --git a/tests/data/acpi/q35/DSDT.acpihmat
> b/tests/data/acpi/q35/DSDT.acpihmat
> index
> 8c1e05a11a328ec1cc6f86e36e52c28f41f9744e..f24d4874bff8d327a165ed7c36de507aea114edd
> 100644
> GIT binary patch
> delta 24
> fcmeD4?(^ny33dtTQ)OUa+&+=(3ZvY{`|DKzU@Hhn
>
> delta 24
> fcmeD4?(^ny33dtTQ)OUa+%}Qx3ZwkS`|DKzU?vDi
>
> diff --git a/tests/data/acpi/q35/DSDT.bridge b/tests/data/acpi/q35/DSDT.bridge
> index
> 6f1464b6c712d7f33cb4b891b7ce76fe228f44c9..424d51bd1cb39ea73501ef7d0044ee52cec5bdac
> 100644
> GIT binary patch
> delta 24
> gcmewz`a6`%CD
> delta 24
> gcmewz`a6`%CD
> diff --git a/tests/data/acpi/q35/DSDT.cphp b/tests/data/acpi/q35/DSDT.cphp
> index
> f8337ff5191a37a47dcf7c09a6c39c4e704a15bf..f1275606f68eeba54bfb11e63d818420385a62b9
> 100644
> GIT binary patch
> delta 24
> fcmdn!veAXhCDF$oF>WH)6-K#@_k$DxTWtqt
>
> delta 24
> fcmdn!veAXhCDF$oF?J%?6-N1u_k$DxTWAMo
>
> diff --git a/tests/data/acpi/q35/DSDT.dimmpxm
> b/tests/data/acpi/q35/DSDT.dimmpxm
> index
> fe5820d93d057ef09a001662369b15afbc5b87e2..76e451e829ec4c245315f7eed8731aa1be45a747
> 100644
> GIT binary patch
> delta 24
> gcmccad)=4ICD
> delta 24
> gcmccad)=4ICD
> diff --git a/tests/data/acpi/q35/DSDT
Re: [PATCH v2 2/2] tests/unit/test-smp-parse: Fix a check-patch complain
On Thu, Nov 11, 2021 at 10:44:29AM +0800, Yanan Wang wrote:
> Checkpatch.pl reports errors like below for commit 9e8e393bb7.
> Let's fix it with a simpler format.
> ERROR: space required after that close brace '}'
> +SMPTestData *data = &(SMPTestData){{ }};
>
> Fixes: 9e8e393bb7 ("tests/unit: Add an unit test for smp parsing")
> Signed-off-by: Yanan Wang
> ---
> tests/unit/test-smp-parse.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index 872512aa37..7805a34b29 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -514,7 +514,7 @@ static void test_generic(void)
> Object *obj = smp_test_machine_init();
> MachineState *ms = MACHINE(obj);
> MachineClass *mc = MACHINE_GET_CLASS(obj);
> -SMPTestData *data = &(SMPTestData){{ }};
> +SMPTestData *data = &(SMPTestData){};
> int i;
>
> for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> @@ -548,7 +548,7 @@ static void test_with_dies(void)
> Object *obj = smp_test_machine_init();
> MachineState *ms = MACHINE(obj);
> MachineClass *mc = MACHINE_GET_CLASS(obj);
> -SMPTestData *data = &(SMPTestData){{ }};
> +SMPTestData *data = &(SMPTestData){};
> unsigned int num_dies = 2;
> int i;
>
> --
> 2.19.1
>
I just did some googling to refresh my memory on this, because I recall
{0} being recommended at some point. From what I can tell, {} is ok for
gcc, maybe also clang, but {0} is ANSI. The reasoning was that {} should
not be empty, and since element names are not required, '0' is enough to
assign the first element to zero. Also, as it's not required to list
each element, then that's enough for the whole struct. That said, {}
seems to be more popular, so we can probably assume the tools we support
also support it.
Reviewed-by: Andrew Jones
Thanks,
drew
Re: [PATCH v2 2/2] tests/unit/test-smp-parse: Fix a check-patch complain
On 11/11/2021 09.43, Andrew Jones wrote:
On Thu, Nov 11, 2021 at 10:44:29AM +0800, Yanan Wang wrote:
Checkpatch.pl reports errors like below for commit 9e8e393bb7.
Let's fix it with a simpler format.
ERROR: space required after that close brace '}'
+SMPTestData *data = &(SMPTestData){{ }};
Fixes: 9e8e393bb7 ("tests/unit: Add an unit test for smp parsing")
Signed-off-by: Yanan Wang
---
tests/unit/test-smp-parse.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 872512aa37..7805a34b29 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -514,7 +514,7 @@ static void test_generic(void)
Object *obj = smp_test_machine_init();
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
-SMPTestData *data = &(SMPTestData){{ }};
+SMPTestData *data = &(SMPTestData){};
int i;
for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
@@ -548,7 +548,7 @@ static void test_with_dies(void)
Object *obj = smp_test_machine_init();
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
-SMPTestData *data = &(SMPTestData){{ }};
+SMPTestData *data = &(SMPTestData){};
unsigned int num_dies = 2;
int i;
--
2.19.1
I just did some googling to refresh my memory on this, because I recall
{0} being recommended at some point. From what I can tell, {} is ok for
gcc, maybe also clang, but {0} is ANSI. The reasoning was that {} should
not be empty, and since element names are not required, '0' is enough to
assign the first element to zero. Also, as it's not required to list
each element, then that's enough for the whole struct. That said, {}
seems to be more popular, so we can probably assume the tools we support
also support it.
Some older compilers did not like {0} (see e.g. commit 555b3d67bc64), that's
why we use {} in the QEMU code IIRC.
Thomas
Re: [RFC PATCH v3 2/2] s390x: Implement the USER_SIGP_BUSY capability
On 10.11.21 21:45, Eric Farman wrote:
> With the USER_SIGP capability, the kernel will pass most (but not all)
> SIGP orders to userspace for processing. But that means that the kernel
> is unable to determine if/when the order has been completed by userspace,
> and could potentially return an incorrect answer (CC1 with status bits
> versus CC2 indicating BUSY) for one of the remaining in-kernel orders.
>
> With a new USER_SIGP_BUSY capability, userspace can tell the kernel when
> it is started processing a SIGP order and when it has finished, such that
> the in-kernel orders can be returned with the BUSY condition between the
> two IOCTLs.
>
> Let's use the new capability in QEMU.
This looks much better. A suggestion on how to make it even simpler below.
> }
> si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
> @@ -375,6 +378,10 @@ static int handle_sigp_single_dst(S390CPU *cpu, S390CPU
> *dst_cpu, uint8_t order,
> return SIGP_CC_BUSY;
> }
>
> +if (s390_cpu_set_sigp_busy(dst_cpu) == -EBUSY) {
> +return SIGP_CC_BUSY;
> +}
I'd assume we want something like this instead:
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -355,6 +355,12 @@ static void sigp_sense_running(S390CPU *dst_cpu, SigpInfo
*si)
}
}
+static bool sigp_dst_is_busy(S390CPU *dst_cpu)
+{
+return dst_cpu->env.sigp_order != 0 ||
+ s390_cpu_set_sigp_busy(dst_cpu) == -EBUSY;
+}
+
static int handle_sigp_single_dst(S390CPU *cpu, S390CPU *dst_cpu, uint8_t
order,
uint64_t param, uint64_t *status_reg)
{
@@ -369,7 +375,7 @@ static int handle_sigp_single_dst(S390CPU *cpu, S390CPU
*dst_cpu, uint8_t order,
}
/* only resets can break pending orders */
-if (dst_cpu->env.sigp_order != 0 &&
+if (sigp_dst_is_busy(dst_cpu) &&
order != SIGP_CPU_RESET &&
order != SIGP_INITIAL_CPU_RESET) {
return SIGP_CC_BUSY;
But as raised, it might be good enough (and simpler) to
special-case SIGP STOP * only, because pending SIGP STOP that could take
forever and is processed asynchronously is AFAIU the only real case that's
problematic. We'll also have to handle the migration case with SIGP STOP,
so below would be my take (excluding the KVM parts from your patch)
diff --git a/slirp b/slirp
--- a/slirp
+++ b/slirp
@@ -1 +1 @@
-Subproject commit a88d9ace234a24ce1c17189642ef9104799425e0
+Subproject commit a88d9ace234a24ce1c17189642ef9104799425e0-dirty
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index ccdbaf84d5..6ead62d1fd 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -114,7 +114,7 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
DeviceState *dev = DEVICE(s);
scc->parent_reset(dev);
-cpu->env.sigp_order = 0;
+s390_cpu_set_sigp_busy(cpu, 0);
s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
switch (type) {
diff --git a/target/s390x/machine.c b/target/s390x/machine.c
index 37a076858c..d4ad2534a5 100644
--- a/target/s390x/machine.c
+++ b/target/s390x/machine.c
@@ -41,6 +41,14 @@ static int cpu_post_load(void *opaque, int version_id)
tcg_s390_tod_updated(CPU(cpu), RUN_ON_CPU_NULL);
}
+/*
+ * Make sure KVM is aware of the BUSY SIGP order, reset it the official
+ * way.
+ */
+if (cpu->env.sigp_order) {
+s390_cpu_set_sigp_busy(cpu, cpu->env.sigp_order);
+}
+
return 0;
}
diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index 1a178aed41..690cadef77 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -402,6 +402,7 @@ void s390x_translate_init(void);
/* sigp.c */
+void s390_cpu_set_sigp_busy(S390CPU *cpu, uint8_t sigp_order);
int handle_sigp(CPUS390XState *env, uint8_t order, uint64_t r1, uint64_t r3);
void do_stop_interrupt(CPUS390XState *env);
diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index 51c727834c..9640267124 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -27,6 +27,33 @@ typedef struct SigpInfo {
uint64_t *status_reg;
} SigpInfo;
+void s390_cpu_set_sigp_busy(S390CPU *cpu, uint8_t sigp_order)
+{
+/*
+ * For now we only expect one of these orders that are processed
+ * asynchronously, or clearing the busy order.
+ */
+g_assert(sigp_order == SIGP_STOP || sigp_order == SIGP_STOP_STORE_STATUS ||
+ !sigp_order);
+if (kvm_enabled()) {
+/*
+ * Note: We're the only ones setting/resetting a CPU in KVM busy, and
+ * we always update the state in KVM when updating the state
+ * (cpu->env.sigp_order) in QEMU.
+ */
+if (sigp_order)
+kvm_s390_vcpu_set_sigp_busy(cpu);
+else
+kvm_s390_vcpu_reset_sigp_busy(cpu);
+}
+cpu->env.sigp_order = sigp_order;
+}
+
+static bool s390x_cpu_is_sigp_busy(S390CPU *cpu)
+{
+return cpu->env.sigp_order != 0;
+}
+
static void set_sigp_status(SigpInfo *si, uint64_t stat
Re: [PATCH v9 3/8] qmp: add QMP command x-query-virtio
On 11/10/21 07:03, Markus Armbruster wrote:
Jonah Palmer writes:
From: Laurent Vivier
This new command lists all the instances of VirtIODevice with
their QOM paths and virtio type/name.
Signed-off-by: Jonah Palmer
[...]
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index 4912b97..1512ada 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -93,3 +93,4 @@
{ 'include': 'audio.json' }
{ 'include': 'acpi.json' }
{ 'include': 'pci.json' }
+{ 'include': 'virtio.json' }
diff --git a/qapi/virtio.json b/qapi/virtio.json
new file mode 100644
index 000..324ba8c
--- /dev/null
+++ b/qapi/virtio.json
@@ -0,0 +1,55 @@
+# -*- Mode: Python -*-
+# vim: filetype=python
+#
+
+##
+# = Virtio devices
+##
+
+##
+# @VirtioInfo:
+#
+# Basic information about a given VirtIODevice
+#
+# @path: the device's canonical QOM path
+#
+# @type: VirtIO device name
+#
+# Since: 6.3
I expect the next release to be numbered 7.0.
Got it. I'll update this for next series.
+#
+##
+{ 'struct': 'VirtioInfo',
+ 'data': { 'path': 'str',
+'type': 'str' } }
+
+##
+# @x-query-virtio:
+#
+# Returns a list of all realized VirtIO devices
+#
+# Features:
+# @unstable: This command is meant for debugging.
+#
+# Returns: list of gathered @VirtioInfo devices
+#
+# Since: 6.3
+#
+# Example:
+#
+# -> { "execute": "x-query-virtio" }
+# <- { "return": [ { "path":
"/machine/peripheral-anon/device[4]/virtio-backend",
+#"type": "virtio-input" },
+# { "path": "/machine/peripheral/crypto0/virtio-backend",
+#"type": "virtio-crypto" },
+# { "path":
"/machine/peripheral-anon/device[2]/virtio-backend",
+#"type": "virtio-scsi" },
+# { "path":
"/machine/peripheral-anon/device[1]/virtio-backend",
+#"type": "virtio-net" },
+# { "path":
"/machine/peripheral-anon/device[0]/virtio-backend",
+#"type": "virtio-serial" }
+#] }
Any particular reason for reformatting the example? For what it's
worth, I'd prefer the previous version.
Aside: consistent formatting of examples would be nice. Not in this
series.
I think I got a little too excited reformatting. I'll revert the examples back
to
their original format and make sure the rest of the examples throughout the
entire
series are consistent with each other.
+#
+##
+
+{ 'command': 'x-query-virtio', 'returns': ['VirtioInfo'],
+ 'features': [ 'unstable' ] }
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index 7f103ea..fd00ee2 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -103,6 +103,7 @@ static bool query_is_ignored(const char *cmd)
"query-gic-capabilities", /* arm */
/* Success depends on target-specific build configuration: */
"query-pci", /* CONFIG_PCI */
+"x-query-virtio", /* CONFIG_VIRTIO */
/* Success depends on launching SEV guest */
"query-sev-launch-measure",
/* Success depends on Host or Hypervisor SEV support */
Re: [PATCH v9 4/8] qmp: add QMP command x-query-virtio-status
On 11/10/21 08:08, Markus Armbruster wrote:
Jonah Palmer writes:
From: Laurent Vivier
This new command shows the status of a VirtIODevice, including
its corresponding vhost device status (if active).
Next patch will improve output by decoding feature bits, including
vhost device's feature bits (backend, protocol, acked, and features).
Also will decode status bits of a VirtIODevice.
Next patch will also suppress the vhost device field from displaying
if no vhost device is active for a given VirtIODevice.
Signed-off-by: Jonah Palmer
[...]
diff --git a/qapi/virtio.json b/qapi/virtio.json
index 324ba8c..54212f2 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -53,3 +53,249 @@
{ 'command': 'x-query-virtio', 'returns': ['VirtioInfo'],
'features': [ 'unstable' ] }
+
+##
+# @VirtioStatusEndianness:
+#
+# Enumeration of endianness for VirtioDevice
+#
+# Since: 6.3
+##
+
+{ 'enum': 'VirtioStatusEndianness',
+ 'data': [ 'unknown', 'little', 'big' ]
+}
+
+##
+# @VhostStatus:
+#
+# Information about a vhost device. This information will only be
+# displayed if the vhost device is active.
+#
+# @n-mem-sections: vhost_dev n_mem_sections
+#
+# @n-tmp-sections: vhost_dev n_tmp_sections
+#
+# @nvqs: vhost_dev nvqs. This is the number of virtqueues being used
+#by the vhost device.
+#
+# @vq-index: vhost_dev vq_index
+#
+# @features: vhost_dev features
+#
+# @acked-features: vhost_dev acked_features
+#
+# @backend-features: vhost_dev backend_features
+#
+# @protocol-features: vhost_dev protocol_features
+#
+# @max-queues: vhost_dev max_queues
+#
+# @backend-cap: vhost_dev backend_cap
+#
+# @log-enabled: vhost_dev log_enabled flag
+#
+# @log-size: vhost_dev log_size
+#
+# Since: 6.3
+#
+##
+
+{ 'struct': 'VhostStatus',
+ 'data': { 'n-mem-sections': 'int',
+'n-tmp-sections': 'int',
+'nvqs': 'uint32',
+'vq-index': 'int',
+'features': 'uint64',
+'acked-features': 'uint64',
+'backend-features': 'uint64',
+'protocol-features': 'uint64',
+'max-queues': 'uint64',
+'backend-cap': 'uint64',
+'log-enabled': 'bool',
+'log-size': 'uint64' } }
+
+##
+# @VirtioStatus:
+#
+# Full status of the virtio device with most VirtIODevice members.
+# Also includes the full status of the corresponding vhost device
+# if the vhost device is active.
+#
+# @name: VirtIODevice name
+#
+# @device-id: VirtIODevice ID
+#
+# @vhost-started: VirtIODevice vhost_started flag
+#
+# @guest-features: VirtIODevice guest_features
+#
+# @host-features: VirtIODevice host_features
+#
+# @backend-features: VirtIODevice backend_features
+#
+# @device-endian: VirtIODevice device_endian
+#
+# @num-vqs: VirtIODevice virtqueue count. This is the number of active
+# virtqueues being used by the VirtIODevice.
+#
+# @status: VirtIODevice configuration status (e.g. DRIVER_OK,
+# FEATURES_OK, DRIVER, etc.)
+#
+# @isr: VirtIODevice ISR
+#
+# @queue-sel: VirtIODevice queue_sel
+#
+# @vm-running: VirtIODevice vm_running flag
+#
+# @broken: VirtIODevice broken flag
+#
+# @disabled: VirtIODevice disabled flag
+#
+# @use-started: VirtIODevice use_started flag
+#
+# @started: VirtIODevice started flag
+#
+# @start-on-kick: VirtIODevice start_on_kick flag
+#
+# @disable-legacy-check: VirtIODevice disabled_legacy_check flag
+#
+# @bus-name: VirtIODevice bus_name
+#
+# @use-guest-notifier-mask: VirtIODevice use_guest_notifier_mask flag
+#
+# @vhost-dev: corresponding vhost device info for a given VirtIODevice
+#
+# Since: 6.3
+#
+##
+
+{ 'struct': 'VirtioStatus',
+ 'data': { 'name': 'str',
+'device-id': 'uint16',
+'vhost-started': 'bool',
+'guest-features': 'uint64',
+'host-features': 'uint64',
+'backend-features': 'uint64',
+'device-endian': 'VirtioStatusEndianness',
+'num-vqs': 'int',
+'status': 'uint8',
+'isr': 'uint8',
+'queue-sel': 'uint16',
+'vm-running': 'bool',
+'broken': 'bool',
+'disabled': 'bool',
+'use-started': 'bool',
+'started': 'bool',
+'start-on-kick': 'bool',
+'disable-legacy-check': 'bool',
+'bus-name': 'str',
+'use-guest-notifier-mask': 'bool',
+'vhost-dev': 'VhostStatus' } }
+
+##
+# @x-query-virtio-status:
+#
+# Poll for a comprehensive status of a given virtio device
+#
+# @path: Canonical QOM path of the VirtIODevice
+#
+# Features:
+# @unstable: This command is meant for debugging.
+#
+# Returns: VirtioStatus of the virtio device
+#
+# Since: 6.3
+#
+# Examples:
+#
+# 1. Poll for the status of virtio-crypto (no vhost-crypto active)
+#
+# -> { "execute": "x-query-virtio-status",
+# "arguments": { "path": "/machine/peripheral/crypto0/virtio-backend" }
+#}
+# <- { "return": {
+# "device-endian": "little",
+# "bus-name
Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing
On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
> From: Yanan Wang
>
> Now that we have a generic parser smp_parse(), let's add an unit
> test for the code. All possible valid/invalid SMP configurations
> that the user can specify are covered.
>
> Signed-off-by: Yanan Wang
> Reviewed-by: Andrew Jones
> Tested-by: Philippe Mathieu-Daudé
> Message-Id: <20211026034659.22040-3-wangyana...@huawei.com>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> tests/unit/test-smp-parse.c | 594
> MAINTAINERS | 1 +
> tests/unit/meson.build | 1 +
> 3 files changed, 596 insertions(+)
> create mode 100644 tests/unit/test-smp-parse.c
> +static struct SMPTestData data_generic_valid[] = {
> +{
> +/* config: no configuration provided
> + * expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */
[1]
> +.config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, F, 0),
> +.expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
> +.expect_prefer_cores = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
> +}, {
> +static void test_generic(void)
> +{
> +Object *obj = object_new(TYPE_MACHINE);
> +MachineState *ms = MACHINE(obj);
> +MachineClass *mc = MACHINE_GET_CLASS(obj);
Watch out, while you create a machine instance in each
test, there is only one machine class registered (see
type_register_static(&smp_machine_info) below in [2]),
...
> +SMPTestData *data = &(SMPTestData){0};
> +int i;
> +
> +smp_machine_class_init(mc);
> +
> +for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> +*data = data_generic_valid[i];
> +unsupported_params_init(mc, data);
> +
> +smp_parse_test(ms, data, true);
> +
> +/* Unsupported parameters can be provided with their values as 1 */
> +data->config.has_dies = true;
> +data->config.dies = 1;
> +smp_parse_test(ms, data, true);
> +}
> +
> +/* Reset the supported min CPUs and max CPUs */
> +mc->min_cpus = 2;
> +mc->max_cpus = 511;
... and here you are modifying the single machine class state, ...
> +
> +for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
> +*data = data_generic_invalid[i];
> +unsupported_params_init(mc, data);
> +
> +smp_parse_test(ms, data, false);
> +}
> +
> +object_unref(obj);
> +}
> +
> +static void test_with_dies(void)
> +{
> +Object *obj = object_new(TYPE_MACHINE);
> +MachineState *ms = MACHINE(obj);
> +MachineClass *mc = MACHINE_GET_CLASS(obj);
... so here the machine class state is inconsistent, ...
> +SMPTestData *data = &(SMPTestData){0};
> +unsigned int num_dies = 2;
> +int i;
> +
> +smp_machine_class_init(mc);
> +mc->smp_props.dies_supported = true;
> +
> +for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> +*data = data_generic_valid[i];
> +unsupported_params_init(mc, data);
> +
> +/* when dies parameter is omitted, it will be set as 1 */
> +data->expect_prefer_sockets.dies = 1;
> +data->expect_prefer_cores.dies = 1;
> +
> +smp_parse_test(ms, data, true);
... in particular the first test [1] is tested with mc->min_cpus = 2.
I wonder why you are not getting:
Output error report: Invalid SMP CPUs 1. The min CPUs supported by
machine '(null)' is 2
for [1].
> +
> +/* when dies parameter is specified */
> +data->config.has_dies = true;
> +data->config.dies = num_dies;
> +if (data->config.has_cpus) {
> +data->config.cpus *= num_dies;
> +}
> +if (data->config.has_maxcpus) {
> +data->config.maxcpus *= num_dies;
> +}
> +
> +data->expect_prefer_sockets.dies = num_dies;
> +data->expect_prefer_sockets.cpus *= num_dies;
> +data->expect_prefer_sockets.max_cpus *= num_dies;
> +data->expect_prefer_cores.dies = num_dies;
> +data->expect_prefer_cores.cpus *= num_dies;
> +data->expect_prefer_cores.max_cpus *= num_dies;
> +
> +smp_parse_test(ms, data, true);
> +}
> +
> +for (i = 0; i < ARRAY_SIZE(data_with_dies_invalid); i++) {
> +*data = data_with_dies_invalid[i];
> +unsupported_params_init(mc, data);
> +
> +smp_parse_test(ms, data, false);
> +}
> +
> +object_unref(obj);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +g_test_init(&argc, &argv, NULL);
> +
> +module_call_init(MODULE_INIT_QOM);
> +type_register_static(&smp_machine_info);
[2]
Re: [PATCH v10 00/10]vhost-vdpa: add support for configure interrupt
On Thu, Nov 11, 2021 at 12:41:02PM +0800, Jason Wang wrote: > > 在 2021/11/8 下午6:53, Stefan Hajnoczi 写道: > > On Fri, Nov 05, 2021 at 12:48:17AM +0800, Cindy Lu wrote: > > > these patches add the support for configure interrupt > > > > > > These codes are all tested in vp-vdpa (support configure interrupt) > > > vdpa_sim (not support configure interrupt), virtio tap device > > > > > > test in virtio-pci bus and virtio-mmio bus > > Hi, > > vhost-user has a configuration space change notification but it uses a > > slave channel message (VHOST_USER_SLAVE_CONFIG_CHANGE_MSG) instead of an > > eventfd. Ideally the vhost kernel ioctl and vhost-user interfaces would > > follow the same design. > > > > I'm concerned "common" vhost code is going to end up with lots of > > callbacks that are not available uniformly across vhost kernel, vdpa, > > and vhost-user. That makes it hard to understand and debug vhost, plus > > differences make it harder to to correctly extend these interfaces in > > the future. > > > > Is the decision to a new eventfd-based interface instead of > > vhost_chr_read/write() deliberate? > > > I think this is a good question. Here're some reasons for using eventfd from > the kernel perspective: > > 1) the eventfd is used for relaying interrupts for vqs, so we choose to use > that for the config interrupt > 2) make it possible to be used for irq bypassing (posted interrupt) Interesting point. Posted interrupts aren't supported by vhost-user's slave channel message. Since configuration change notifications are rare it's probably not a performance problem, but still. This makes me think vhost-user's approach is sub-optimal, it should have been an eventfd :(. Maybe the idea was that a slave message is less complex than adding an additional interface to set a configuration change notification eventfd. Let's not worry about it too much. I guess in the long run vhost-user can be rebased on top of the vDPA kernel interface (I wrote about that here: https://blog.vmsplice.net/2020/09/on-unifying-vhost-user-and-virtio.html). Stefan signature.asc Description: PGP signature
Re: [PATCH v2 1/6] qapi/qom,target/i386: sev-guest: Introduce kernel-hashes=on|off option
On Mon, Nov 08, 2021 at 08:20:48PM +0200, Dov Murik wrote: > > > On 08/11/2021 17:51, Markus Armbruster wrote: > > Dov Murik writes: > > > >> Introduce new boolean 'kernel-hashes' option on the sev-guest object. > >> It will be used to to decide whether to add the hashes of > >> kernel/initrd/cmdline to SEV guest memory when booting with -kernel. > >> The default value is 'off'. > >> > >> Signed-off-by: Dov Murik > >> --- > >> qapi/qom.json | 7 ++- > >> target/i386/sev.c | 20 > >> qemu-options.hx | 6 +- > >> 3 files changed, 31 insertions(+), 2 deletions(-) > >> > >> diff --git a/qapi/qom.json b/qapi/qom.json > >> index ccd1167808..4fd5d1716b 100644 > >> --- a/qapi/qom.json > >> +++ b/qapi/qom.json > >> @@ -769,6 +769,10 @@ > >> # @reduced-phys-bits: number of bits in physical addresses that become > >> # unavailable when SEV is enabled > >> # > >> +# @kernel-hashes: if true, add hashes of kernel/initrd/cmdline to a > >> +# designated guest firmware page for measured boot > >> +# with -kernel (default: false) > > > > Missing: (since 7.0) > > > > I agree the "since" clause is missing, but I think this series (at least > patches 1-4) should be considered a bug fix (because booting with > -kernel will break in 6.2 for older OVMF which doesn't have guest > firmware area for hashes). > > I think it should be added for 6.2. > > Paolo? > > > If agreed, the hunk should be: Yes, the kernel hashes feature was introduced in this 6.2 dev cycle, and this patch is fixing a significant behavioural problem with it. We need this included in the 6.2 release Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH for 6.2 v2 5/5] bios-tables-test: Update golden binaries
On Thu, 11 Nov 2021, Michael S. Tsirkin wrote:
> On Wed, Nov 10, 2021 at 04:11:40PM -0500, Igor Mammedov wrote:
> > From: Julia Suvorova
> >
> > The changes are the result of
> > 'hw/i386/acpi-build: Deny control on PCIe Native Hot-Plug in _OSC'
> > and listed here:
> >
> > Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities
> > {
> > CreateDWordField (Arg3, Zero, CDW1)
> > If ((Arg0 == ToUUID
> > ("33db4d5b-1ff7-401c-9657-7441c03dd766") /* PCI Host Bridge Device */))
> > {
> > CreateDWordField (Arg3, 0x04, CDW2)
> > CreateDWordField (Arg3, 0x08, CDW3)
> > Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> > -Local0 &= 0x1F
> > +Local0 &= 0x1E
> >
> > Signed-off-by: Julia Suvorova
> > Signed-off-by: Igor Mammedov
> > ---
> > tests/qtest/bios-tables-test-allowed-diff.h | 16
> > tests/data/acpi/q35/DSDT| Bin 8289 -> 8289 bytes
> > tests/data/acpi/q35/DSDT.acpihmat | Bin 9614 -> 9614 bytes
> > tests/data/acpi/q35/DSDT.bridge | Bin 11003 -> 11003 bytes
> > tests/data/acpi/q35/DSDT.cphp | Bin 8753 -> 8753 bytes
> > tests/data/acpi/q35/DSDT.dimmpxm| Bin 9943 -> 9943 bytes
> > tests/data/acpi/q35/DSDT.dmar | Bin 0 -> 8289 bytes
> > tests/data/acpi/q35/DSDT.ipmibt | Bin 8364 -> 8364 bytes
> > tests/data/acpi/q35/DSDT.ivrs | Bin 8306 -> 8306 bytes
> > tests/data/acpi/q35/DSDT.memhp | Bin 9648 -> 9648 bytes
> > tests/data/acpi/q35/DSDT.mmio64 | Bin 9419 -> 9419 bytes
> > tests/data/acpi/q35/DSDT.multi-bridge | Bin 8583 -> 8583 bytes
> > tests/data/acpi/q35/DSDT.nohpet | Bin 8147 -> 8147 bytes
> > tests/data/acpi/q35/DSDT.nosmm | Bin 0 -> 8289 bytes
> > tests/data/acpi/q35/DSDT.numamem| Bin 8295 -> 8295 bytes
> > tests/data/acpi/q35/DSDT.smm-compat | Bin 0 -> 8289 bytes
> > tests/data/acpi/q35/DSDT.smm-compat-nosmm | Bin 0 -> 8289 bytes
> > tests/data/acpi/q35/DSDT.tis.tpm12 | Bin 8894 -> 8894 bytes
> > tests/data/acpi/q35/DSDT.tis.tpm2 | Bin 8894 -> 8894 bytes
> > tests/data/acpi/q35/DSDT.xapic | Bin 35652 -> 35652 bytes
>
> Why do we have all the new files? What is going on here?
Good catch. I saw those files even in my workspace and failed to notice
that they were being newly created in this patch and they did not exist
previously:
https://git.qemu.org/?p=qemu.git;a=tree;f=tests/data/acpi/q35;h=e9d1edd2671997a3e7fe278018313bcbfcfb0850;hb=HEAD
>
> > 20 files changed, 16 deletions(-)
> > create mode 100644 tests/data/acpi/q35/DSDT.dmar
The corresponding change that adds the test is :
commit 0ff92b6d99011c8de57321503c0eb655c461a217
Author: Igor Mammedov
Date: Thu Sep 2 07:35:43 2021 -0400
tests: acpi: add testcase for intel_iommu (DMAR table)
Igor has updated the DMAR table blob here:
commit 44d3bdd8a6f1ae2a5ca417251736a033900d4c08
Author: Igor Mammedov
Date: Thu Sep 2 07:35:44 2021 -0400
tests: acpi: add expected blob for DMAR table
but maybe the test also introduced changes in DSDT table as well?
Needs more investigation.
> > create mode 100644 tests/data/acpi/q35/DSDT.nosmm
> > create mode 100644 tests/data/acpi/q35/DSDT.smm-compat
> > create mode 100644 tests/data/acpi/q35/DSDT.smm-compat-nosmm
The corresponding tests for these files were added in this commit:
commit 0dabb2e802437c2e578dc72bd0bdf3380a25ec96
Author: Isaku Yamahata
Date: Wed Feb 17 21:51:14 2021 -0800
BUT it seems that by mistake the table blobs were not added when the
following commit was made:
commit 7b630d937a6c73fb145746fb31e0fb4b08f0cf0e
Author: Isaku Yamahata
Date: Wed Feb 17 21:51:18 2021 -0800
qtest/acpi/bios-tables-test: update acpi tables
Sadly, the above commit does update tests/data/acpi/q35/DSDT which I
believe includes the changes for the above tests. The commit message
does not add the ASL diff which the tests introduces.
I believe at some point I did TEST_ACPI_REBUILD_AML which regenrated those
files in my workspace and hence they were there. Same could be true for
Igor and got added when he generated the commit.
> >
> > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h
> > b/tests/qtest/bios-tables-test-allowed-diff.h
> > index 48e5634d4b..dfb8523c8b 100644
> > --- a/tests/qtest/bios-tables-test-allowed-diff.h
> > +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> > @@ -1,17 +1 @@
> > /* List of comma-separated changed AML files to ignore */
> > -"tests/data/acpi/q35/DSDT",
> > -"tests/data/acpi/q35/DSDT.tis",
> > -"tests/data/acpi/q35/DSDT.bridge",
> > -"tests/data/acpi/q35/DSDT.mmio64",
> > -"tests/data/acpi/q35/DSDT.ipmibt",
> > -"tests/data/acpi/q35/DSDT.cphp",
> > -"tests/data/acpi/q35/DSDT.memhp",
> > -"tests/data/acpi/q35/DS
Re: [PATCH v2 1/6] qapi/qom,target/i386: sev-guest: Introduce kernel-hashes=on|off option
On Mon, Nov 08, 2021 at 01:48:35PM +, Dov Murik wrote: > Introduce new boolean 'kernel-hashes' option on the sev-guest object. > It will be used to to decide whether to add the hashes of > kernel/initrd/cmdline to SEV guest memory when booting with -kernel. > The default value is 'off'. > > Signed-off-by: Dov Murik > --- > qapi/qom.json | 7 ++- > target/i386/sev.c | 20 > qemu-options.hx | 6 +- > 3 files changed, 31 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé (Assuming the version tag is added as discussed) Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 1/3] target/ppc: Implement Vector Expand Mask
On 11/10/21 7:56 PM, matheus.fe...@eldorado.org.br wrote: From: Matheus Ferst Implement the following PowerISA v3.1 instructions: vexpandbm: Vector Expand Byte Mask vexpandhm: Vector Expand Halfword Mask vexpandwm: Vector Expand Word Mask vexpanddm: Vector Expand Doubleword Mask vexpandqm: Vector Expand Quadword Mask Signed-off-by: Matheus Ferst --- target/ppc/insn32.decode| 11 ++ target/ppc/translate/vmx-impl.c.inc | 34 + 2 files changed, 45 insertions(+) Reviewed-by: Richard Henderson r~
Re: [PATCH v2 2/6] target/i386/sev: Add kernel hashes only if sev-guest.kernel-hashes=on
On Mon, Nov 08, 2021 at 01:48:36PM +, Dov Murik wrote:
> Commit cff03145ed3c ("sev/i386: Introduce sev_add_kernel_loader_hashes
> for measured linux boot", 2021-09-30) introduced measured direct boot
> with -kernel, using an OVMF-designated hashes table which QEMU fills.
>
> However, if OVMF doesn't designate such an area, QEMU would completely
> abort the VM launch. This breaks launching with -kernel using older
> OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID.
>
> Fix that so QEMU will only look for the hashes table if the sev-guest
> kernel-hashes option is set to on. Otherwise, QEMU won't look for the
> designated area in OVMF and won't fill that area.
>
> To enable addition of kernel hashes, launch the guest with:
>
> -object sev-guest,...,kernel-hashes=on
>
> Signed-off-by: Dov Murik
> Reported-by: Tom Lendacky
> ---
> target/i386/sev.c | 8
> 1 file changed, 8 insertions(+)
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing
On 2021/11/11 17:14, Philippe Mathieu-Daudé wrote:
On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
From: Yanan Wang
Now that we have a generic parser smp_parse(), let's add an unit
test for the code. All possible valid/invalid SMP configurations
that the user can specify are covered.
Signed-off-by: Yanan Wang
Reviewed-by: Andrew Jones
Tested-by: Philippe Mathieu-Daudé
Message-Id: <20211026034659.22040-3-wangyana...@huawei.com>
Signed-off-by: Philippe Mathieu-Daudé
---
tests/unit/test-smp-parse.c | 594
MAINTAINERS | 1 +
tests/unit/meson.build | 1 +
3 files changed, 596 insertions(+)
create mode 100644 tests/unit/test-smp-parse.c
+static struct SMPTestData data_generic_valid[] = {
+{
+/* config: no configuration provided
+ * expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */
[1]
+.config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, F, 0),
+.expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
+.expect_prefer_cores = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
+}, {
+static void test_generic(void)
+{
+Object *obj = object_new(TYPE_MACHINE);
+MachineState *ms = MACHINE(obj);
+MachineClass *mc = MACHINE_GET_CLASS(obj);
Watch out, while you create a machine instance in each
test, there is only one machine class registered (see
type_register_static(&smp_machine_info) below in [2]),
...
Yes, I noticed this. So on the top of each sub-test function, the properties
of the single machine class is re-initialized by smp_machine_class_init(mc).
See [*] below.
+SMPTestData *data = &(SMPTestData){0};
+int i;
+
+smp_machine_class_init(mc);
[*]
+
+for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
+*data = data_generic_valid[i];
+unsupported_params_init(mc, data);
+
+smp_parse_test(ms, data, true);
+
+/* Unsupported parameters can be provided with their values as 1 */
+data->config.has_dies = true;
+data->config.dies = 1;
+smp_parse_test(ms, data, true);
+}
+
+/* Reset the supported min CPUs and max CPUs */
+mc->min_cpus = 2;
+mc->max_cpus = 511;
... and here you are modifying the single machine class state, ...
+
+for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
+*data = data_generic_invalid[i];
+unsupported_params_init(mc, data);
+
+smp_parse_test(ms, data, false);
+}
+
+object_unref(obj);
+}
+
+static void test_with_dies(void)
+{
+Object *obj = object_new(TYPE_MACHINE);
+MachineState *ms = MACHINE(obj);
+MachineClass *mc = MACHINE_GET_CLASS(obj);
... so here the machine class state is inconsistent, ...
+SMPTestData *data = &(SMPTestData){0};
+unsigned int num_dies = 2;
+int i;
+
+smp_machine_class_init(mc);
And here [*].
+mc->smp_props.dies_supported = true;
+
+for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
+*data = data_generic_valid[i];
+unsupported_params_init(mc, data);
+
+/* when dies parameter is omitted, it will be set as 1 */
+data->expect_prefer_sockets.dies = 1;
+data->expect_prefer_cores.dies = 1;
+
+smp_parse_test(ms, data, true);
... in particular the first test [1] is tested with mc->min_cpus = 2.
I wonder why you are not getting:
Output error report: Invalid SMP CPUs 1. The min CPUs supported by
machine '(null)' is 2
for [1].
So as I have explained above, we won't get an output error report like
this here. :)
Thanks,
Yanan
+
+/* when dies parameter is specified */
+data->config.has_dies = true;
+data->config.dies = num_dies;
+if (data->config.has_cpus) {
+data->config.cpus *= num_dies;
+}
+if (data->config.has_maxcpus) {
+data->config.maxcpus *= num_dies;
+}
+
+data->expect_prefer_sockets.dies = num_dies;
+data->expect_prefer_sockets.cpus *= num_dies;
+data->expect_prefer_sockets.max_cpus *= num_dies;
+data->expect_prefer_cores.dies = num_dies;
+data->expect_prefer_cores.cpus *= num_dies;
+data->expect_prefer_cores.max_cpus *= num_dies;
+
+smp_parse_test(ms, data, true);
+}
+
+for (i = 0; i < ARRAY_SIZE(data_with_dies_invalid); i++) {
+*data = data_with_dies_invalid[i];
+unsupported_params_init(mc, data);
+
+smp_parse_test(ms, data, false);
+}
+
+object_unref(obj);
+}
+
+int main(int argc, char *argv[])
+{
+g_test_init(&argc, &argv, NULL);
+
+module_call_init(MODULE_INIT_QOM);
+type_register_static(&smp_machine_info);
[2]
.
Re: [PATCH v2 5/6] target/i386/sev: Perform padding calculations at compile-time
On Mon, Nov 08, 2021 at 01:48:39PM +, Dov Murik wrote: > In sev_add_kernel_loader_hashes, the sizes of structs are known at > compile-time, so calculate needed padding at compile-time. > > No functional change intended. > > Signed-off-by: Dov Murik > Reviewed-by: Dr. David Alan Gilbert > Reviewed-by: Philippe Mathieu-Daudé > --- > target/i386/sev.c | 28 ++-- > 1 file changed, 18 insertions(+), 10 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 4/6] target/i386/sev: Fail when invalid hashes table area detected
On Mon, Nov 08, 2021 at 01:48:38PM +, Dov Murik wrote:
> Commit cff03145ed3c ("sev/i386: Introduce sev_add_kernel_loader_hashes
> for measured linux boot", 2021-09-30) introduced measured direct boot
> with -kernel, using an OVMF-designated hashes table which QEMU fills.
>
> However, no checks are performed on the validity of the hashes area
> designated by OVMF. Specifically, if OVMF publishes the
> SEV_HASH_TABLE_RV_GUID entry but it is filled with zeroes, this will
> cause QEMU to write the hashes entries over the first page of the
> guest's memory (GPA 0).
>
> Add validity checks to the published area. If the hashes table area's
> base address is zero, or its size is too small to fit the aligned hashes
> table, display an error and stop the guest launch. In such case, the
> following error will be displayed:
>
> qemu-system-x86_64: SEV: guest firmware hashes table area is invalid
> (base=0x0 size=0x0)
>
> Signed-off-by: Dov Murik
> Reported-by: Brijesh Singh
> ---
> target/i386/sev.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 6/6] target/i386/sev: Replace qemu_map_ram_ptr with address_space_map
On Mon, Nov 08, 2021 at 01:48:40PM +, Dov Murik wrote: > Use address_space_map/unmap and check for errors. > > Signed-off-by: Dov Murik > --- > target/i386/sev.c | 16 +--- > 1 file changed, 13 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports
On Thu, Nov 11, 2021 at 03:20:07AM -0500, Michael S. Tsirkin wrote: > On Thu, Nov 11, 2021 at 08:53:06AM +0100, Gerd Hoffmann wrote: > > Hi, > > > > > Given it's a bugfix, and given that I hear through internal channels > > > that QE results so far have been encouraging, I am inclined to bite the > > > bullet and merge this for -rc1. > > > > Fine with me. > > > > > I don't think this conflicts with Julia's patches as users can still > > > disable ACPI hotplug into bridges. Gerd, agree? Worth the risk? > > > > Combining this with Julia's patches looks a bit risky to me and surely > > needs testing. I expect the problematic case is both native and acpi > > hotplug being enabled. > > When the guest uses acpi hotplug nobody will > > turn on slot power on the pcie root port ... > > I'm not sure I understand what the situation is, and how to trigger it. > Could you clarify pls? My patch series implements proper slot power emulation for pcie root ports, i.e. the pci device plugged into the slot is only visible to the guest when slot power is enabled. When the pciehp driver is used the linux kernel will enable slot power of course. When the acpihp driver is used the linux kernel will just call the aml methods and I suspect the pci device will stay invisible then because nobody flips the slot power control bit (with native-hotplug=on, for native-hotplug=off this isn't a problem of course). > > I'd suggest to just revert to pcie native hotplug for 6.2. > > Hmm that kind of change seems even riskier to me. It's not clear to me why you consider that riskier. It should fix the qemu 6.1 regressions. Given QE has found no problems so far it should not be worse than qemu 6.0 was. Most likely it will work better than qemu 6.0. Going with a new approach (enable both native and acpi hotplug) after freeze looks risky to me, even without considering the conflict outlined above. Last-minute changes with the chance to repeat the 6.1 disaster, only with a different set of bugs ... take care, Gerd
Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports
On Thu, Nov 11, 2021 at 03:20:07AM -0500, Michael S. Tsirkin wrote: > On Thu, Nov 11, 2021 at 08:53:06AM +0100, Gerd Hoffmann wrote: > > Hi, > > > > > Given it's a bugfix, and given that I hear through internal channels > > > that QE results so far have been encouraging, I am inclined to bite the > > > bullet and merge this for -rc1. > > > > Fine with me. > > > > > I don't think this conflicts with Julia's patches as users can still > > > disable ACPI hotplug into bridges. Gerd, agree? Worth the risk? > > > > Combining this with Julia's patches looks a bit risky to me and surely > > needs testing. I expect the problematic case is both native and acpi > > hotplug being enabled. > > When the guest uses acpi hotplug nobody will > > turn on slot power on the pcie root port ... > > I'm not sure I understand what the situation is, and how to trigger it. > Could you clarify pls? > > > I'd suggest to just revert to pcie native hotplug for 6.2. > > Hmm that kind of change seems even riskier to me. I think I'll try with > Igor's patches. Why would it be risky ? PCIE native hotplug is what we've used in QEMU for years & years, until 6.1 enabled the buggy ACPI hotplug. The behaviour of the current PCIE native hotplug impl is a known quantity. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing
On 11/11/21 10:31, wangyanan (Y) wrote:
>
> On 2021/11/11 17:14, Philippe Mathieu-Daudé wrote:
>> On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
>>> From: Yanan Wang
>>>
>>> Now that we have a generic parser smp_parse(), let's add an unit
>>> test for the code. All possible valid/invalid SMP configurations
>>> that the user can specify are covered.
>>>
>>> Signed-off-by: Yanan Wang
>>> Reviewed-by: Andrew Jones
>>> Tested-by: Philippe Mathieu-Daudé
>>> Message-Id: <20211026034659.22040-3-wangyana...@huawei.com>
>>> Signed-off-by: Philippe Mathieu-Daudé
>>> ---
>>> tests/unit/test-smp-parse.c | 594
>>> MAINTAINERS | 1 +
>>> tests/unit/meson.build | 1 +
>>> 3 files changed, 596 insertions(+)
>>> create mode 100644 tests/unit/test-smp-parse.c
>>> +static struct SMPTestData data_generic_valid[] = {
>>> + {
>>> + /* config: no configuration provided
>>> + * expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */
>> [1]
>>
>>> + .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, F, 0),
>>> + .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
>>> + .expect_prefer_cores = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
>>> + }, {
>>> +static void test_generic(void)
>>> +{
>>> + Object *obj = object_new(TYPE_MACHINE);
>>> + MachineState *ms = MACHINE(obj);
>>> + MachineClass *mc = MACHINE_GET_CLASS(obj);
>> Watch out, while you create a machine instance in each
>> test, there is only one machine class registered (see
>> type_register_static(&smp_machine_info) below in [2]),
>> ...
> Yes, I noticed this. So on the top of each sub-test function, the
> properties
> of the single machine class is re-initialized by
> smp_machine_class_init(mc).
> See [*] below.
>>> + SMPTestData *data = &(SMPTestData){0};
>>> + int i;
>>> +
>>> + smp_machine_class_init(mc);
> [*]
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
>>> + *data = data_generic_valid[i];
>>> + unsupported_params_init(mc, data);
>>> +
>>> + smp_parse_test(ms, data, true);
>>> +
>>> + /* Unsupported parameters can be provided with their values
>>> as 1 */
>>> + data->config.has_dies = true;
>>> + data->config.dies = 1;
>>> + smp_parse_test(ms, data, true);
>>> + }
>>> +
>>> + /* Reset the supported min CPUs and max CPUs */
>>> + mc->min_cpus = 2;
>>> + mc->max_cpus = 511;
>> ... and here you are modifying the single machine class state, ...
>>
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
>>> + *data = data_generic_invalid[i];
>>> + unsupported_params_init(mc, data);
>>> +
>>> + smp_parse_test(ms, data, false);
>>> + }
>>> +
>>> + object_unref(obj);
>>> +}
>>> +
>>> +static void test_with_dies(void)
>>> +{
>>> + Object *obj = object_new(TYPE_MACHINE);
>>> + MachineState *ms = MACHINE(obj);
>>> + MachineClass *mc = MACHINE_GET_CLASS(obj);
>> ... so here the machine class state is inconsistent, ...
>>
>>> + SMPTestData *data = &(SMPTestData){0};
>>> + unsigned int num_dies = 2;
>>> + int i;
>>> +
>>> + smp_machine_class_init(mc);
> And here [*].
>>> + mc->smp_props.dies_supported = true;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
>>> + *data = data_generic_valid[i];
>>> + unsupported_params_init(mc, data);
>>> +
>>> + /* when dies parameter is omitted, it will be set as 1 */
>>> + data->expect_prefer_sockets.dies = 1;
>>> + data->expect_prefer_cores.dies = 1;
>>> +
>>> + smp_parse_test(ms, data, true);
>> ... in particular the first test [1] is tested with mc->min_cpus = 2.
>>
>> I wonder why you are not getting:
>>
>> Output error report: Invalid SMP CPUs 1. The min CPUs supported by
>> machine '(null)' is 2
>>
>> for [1].
> So as I have explained above, we won't get an output error report like
> this here. :)
I see. IMHO this is bad practice example, so I'll send a cleanup patch.
Re: [PATCH v2 1/6] qapi/qom,target/i386: sev-guest: Introduce kernel-hashes=on|off option
On 11/11/2021 11:26, Daniel P. Berrangé wrote: > On Mon, Nov 08, 2021 at 08:20:48PM +0200, Dov Murik wrote: >> >> >> On 08/11/2021 17:51, Markus Armbruster wrote: >>> Dov Murik writes: >>> Introduce new boolean 'kernel-hashes' option on the sev-guest object. It will be used to to decide whether to add the hashes of kernel/initrd/cmdline to SEV guest memory when booting with -kernel. The default value is 'off'. Signed-off-by: Dov Murik --- qapi/qom.json | 7 ++- target/i386/sev.c | 20 qemu-options.hx | 6 +- 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/qapi/qom.json b/qapi/qom.json index ccd1167808..4fd5d1716b 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -769,6 +769,10 @@ # @reduced-phys-bits: number of bits in physical addresses that become # unavailable when SEV is enabled # +# @kernel-hashes: if true, add hashes of kernel/initrd/cmdline to a +# designated guest firmware page for measured boot +# with -kernel (default: false) >>> >>> Missing: (since 7.0) >>> >> >> I agree the "since" clause is missing, but I think this series (at least >> patches 1-4) should be considered a bug fix (because booting with >> -kernel will break in 6.2 for older OVMF which doesn't have guest >> firmware area for hashes). >> >> I think it should be added for 6.2. >> >> Paolo? >> >> >> If agreed, the hunk should be: > > Yes, the kernel hashes feature was introduced in this 6.2 dev > cycle, and this patch is fixing a significant behavioural > problem with it. We need this included in the 6.2 release > Thanks for reviewing. I'll shortly send a v3 with the minor doc/string changes suggested here (patches 1 and 3). -Dov
Re: [PATCH v2 0/6] SEV: add kernel-hashes=on for measured -kernel launch
On Mon, Nov 08, 2021 at 01:48:34PM +, Dov Murik wrote: > Tom Lendacky and Brijesh Singh reported two issues with launching SEV > guests with the -kernel QEMU option when an old [1] or wrongly configured [2] > OVMF images are used. > > To fix these issues, these series "hides" the whole kernel hashes > additions behind a kernel-hashes=on option (with default value of > "off"). This allows existing scenarios to work without change, and > explicitly forces kernel hashes additions for guests that require that. We need to to get this into 6.2 to adress the regression vs previous QEMU releases. There's just a couple of small review points. If you can repost with the easy fixes, then we can get this queued. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH for 6.2 v2 5/5] bios-tables-test: Update golden binaries
On Thu, 11 Nov 2021, Ani Sinha wrote:
>
>
> On Thu, 11 Nov 2021, Michael S. Tsirkin wrote:
>
> > On Wed, Nov 10, 2021 at 04:11:40PM -0500, Igor Mammedov wrote:
> > > From: Julia Suvorova
> > >
> > > The changes are the result of
> > > 'hw/i386/acpi-build: Deny control on PCIe Native Hot-Plug in _OSC'
> > > and listed here:
> > >
> > > Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities
> > > {
> > > CreateDWordField (Arg3, Zero, CDW1)
> > > If ((Arg0 == ToUUID
> > > ("33db4d5b-1ff7-401c-9657-7441c03dd766") /* PCI Host Bridge Device */))
> > > {
> > > CreateDWordField (Arg3, 0x04, CDW2)
> > > CreateDWordField (Arg3, 0x08, CDW3)
> > > Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> > > -Local0 &= 0x1F
> > > +Local0 &= 0x1E
> > >
> > > Signed-off-by: Julia Suvorova
> > > Signed-off-by: Igor Mammedov
> > > ---
> > > tests/qtest/bios-tables-test-allowed-diff.h | 16
> > > tests/data/acpi/q35/DSDT| Bin 8289 -> 8289 bytes
> > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9614 -> 9614 bytes
> > > tests/data/acpi/q35/DSDT.bridge | Bin 11003 -> 11003 bytes
> > > tests/data/acpi/q35/DSDT.cphp | Bin 8753 -> 8753 bytes
> > > tests/data/acpi/q35/DSDT.dimmpxm| Bin 9943 -> 9943 bytes
> > > tests/data/acpi/q35/DSDT.dmar | Bin 0 -> 8289 bytes
> > > tests/data/acpi/q35/DSDT.ipmibt | Bin 8364 -> 8364 bytes
> > > tests/data/acpi/q35/DSDT.ivrs | Bin 8306 -> 8306 bytes
> > > tests/data/acpi/q35/DSDT.memhp | Bin 9648 -> 9648 bytes
> > > tests/data/acpi/q35/DSDT.mmio64 | Bin 9419 -> 9419 bytes
> > > tests/data/acpi/q35/DSDT.multi-bridge | Bin 8583 -> 8583 bytes
> > > tests/data/acpi/q35/DSDT.nohpet | Bin 8147 -> 8147 bytes
> > > tests/data/acpi/q35/DSDT.nosmm | Bin 0 -> 8289 bytes
> > > tests/data/acpi/q35/DSDT.numamem| Bin 8295 -> 8295 bytes
> > > tests/data/acpi/q35/DSDT.smm-compat | Bin 0 -> 8289 bytes
> > > tests/data/acpi/q35/DSDT.smm-compat-nosmm | Bin 0 -> 8289 bytes
> > > tests/data/acpi/q35/DSDT.tis.tpm12 | Bin 8894 -> 8894 bytes
> > > tests/data/acpi/q35/DSDT.tis.tpm2 | Bin 8894 -> 8894 bytes
> > > tests/data/acpi/q35/DSDT.xapic | Bin 35652 -> 35652 bytes
> >
> > Why do we have all the new files? What is going on here?
>
> Good catch. I saw those files even in my workspace and failed to notice
> that they were being newly created in this patch and they did not exist
> previously:
> https://git.qemu.org/?p=qemu.git;a=tree;f=tests/data/acpi/q35;h=e9d1edd2671997a3e7fe278018313bcbfcfb0850;hb=HEAD
>
>
> >
> > > 20 files changed, 16 deletions(-)
> > > create mode 100644 tests/data/acpi/q35/DSDT.dmar
>
> The corresponding change that adds the test is :
>
> commit 0ff92b6d99011c8de57321503c0eb655c461a217
> Author: Igor Mammedov
> Date: Thu Sep 2 07:35:43 2021 -0400
>
> tests: acpi: add testcase for intel_iommu (DMAR table)
>
> Igor has updated the DMAR table blob here:
>
> commit 44d3bdd8a6f1ae2a5ca417251736a033900d4c08
> Author: Igor Mammedov
> Date: Thu Sep 2 07:35:44 2021 -0400
>
> tests: acpi: add expected blob for DMAR table
>
> but maybe the test also introduced changes in DSDT table as well?
> Needs more investigation.
Actually I am pretty sure that no changes in DSDT was introduced with the
above test. If it did, the tests would be broken because Igor did not
update the DSDT table blob. So we should not commit the
tests/data/acpi/q35/DSDT.dmar
table blob here.
>
> > > create mode 100644 tests/data/acpi/q35/DSDT.nosmm
> > > create mode 100644 tests/data/acpi/q35/DSDT.smm-compat
> > > create mode 100644 tests/data/acpi/q35/DSDT.smm-compat-nosmm
>
> The corresponding tests for these files were added in this commit:
>
> commit 0dabb2e802437c2e578dc72bd0bdf3380a25ec96
> Author: Isaku Yamahata
> Date: Wed Feb 17 21:51:14 2021 -0800
>
> BUT it seems that by mistake the table blobs were not added when the
> following commit was made:
>
> commit 7b630d937a6c73fb145746fb31e0fb4b08f0cf0e
> Author: Isaku Yamahata
> Date: Wed Feb 17 21:51:18 2021 -0800
>
> qtest/acpi/bios-tables-test: update acpi tables
>
> Sadly, the above commit does update tests/data/acpi/q35/DSDT which I
> believe includes the changes for the above tests. The commit message
> does not add the ASL diff which the tests introduces.
We need to check this one. If the above commit did indeed introduce
changes in DSDT blob which it should have done tgrough those additional
table blob files.
>
> I believe at some point I did TEST_ACPI_REBUILD_AML which regenrated those
> files in my workspace and hence they were there. Same could be true for
> Igor and got added when he generated the commit.
>
[PATCH] q35: flip acpi-pci-hotplug-with-bridge-support default back to off
Switch qemu 6.2 back to 6.0 behavior (aka native pcie hotplug) because
acpi hotplug for pcie ports caused all kinds of regressions and a fix
for those is not in sight.
Add compat property for 6.1 to keep it enabled there. Use a separate
compat property list so we can apply it to 6.1 only.
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/641
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2006409
Signed-off-by: Gerd Hoffmann
---
hw/acpi/ich9.c | 2 +-
hw/i386/pc.c | 1 -
hw/i386/pc_q35.c | 14 +-
3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 1ee2ba2c508c..6e7d4c9eb54a 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -427,7 +427,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
pm->disable_s3 = 0;
pm->disable_s4 = 0;
pm->s4_val = 2;
-pm->use_acpi_hotplug_bridge = true;
+pm->use_acpi_hotplug_bridge = false;
object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
&pm->pm_io_base, OBJ_PROP_FLAG_READ);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2592a821486f..4fed82dafcf0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -106,7 +106,6 @@ GlobalProperty pc_compat_6_0[] = {
{ "qemu64" "-" TYPE_X86_CPU, "model", "6" },
{ "qemu64" "-" TYPE_X86_CPU, "stepping", "3" },
{ TYPE_X86_CPU, "x-vendor-cpuid-only", "off" },
-{ "ICH9-LPC", ACPI_PM_PROP_ACPI_PCIHP_BRIDGE, "off" },
};
const size_t pc_compat_6_0_len = G_N_ELEMENTS(pc_compat_6_0);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 797e09500b15..735dd3cff4ed 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -375,8 +375,20 @@ static void pc_q35_6_1_machine_options(MachineClass *m)
m->smp_props.prefer_sockets = true;
}
+/* 6.1 only compat property (not applied to 6.0 + older) */
+static GlobalProperty pc_compat_6_1_only[] = {
+{ "ICH9-LPC", ACPI_PM_PROP_ACPI_PCIHP_BRIDGE, "on" },
+};
+static const size_t pc_compat_6_1_only_len = G_N_ELEMENTS(pc_compat_6_1_only);
+
+static void pc_q35_6_1_only_machine_options(MachineClass *m)
+{
+pc_q35_6_1_machine_options(m);
+compat_props_add(m->compat_props, pc_compat_6_1_only,
pc_compat_6_1_only_len);
+}
+
DEFINE_Q35_MACHINE(v6_1, "pc-q35-6.1", NULL,
- pc_q35_6_1_machine_options);
+ pc_q35_6_1_only_machine_options);
static void pc_q35_6_0_machine_options(MachineClass *m)
{
--
2.33.1
[PATCH v2 1/3] icount: preserve cflags when custom tb is about to execute
When debugging with the watchpoints, qemu may need to create
TB with single instruction. This is achieved by setting cpu->cflags_next_tb.
But when this block is about to execute, it may be interrupted by another
thread. In this case cflags will be lost and next executed TB will not
be the special one.
This patch checks TB exit reason and restores cflags_next_tb to allow
finding the interrupted block.
Signed-off-by: Pavel Dovgalyuk
---
accel/tcg/cpu-exec.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 2d14d02f6c..df12452b8f 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -846,6 +846,16 @@ static inline void cpu_loop_exec_tb(CPUState *cpu,
TranslationBlock *tb,
* cpu_handle_interrupt. cpu_handle_interrupt will also
* clear cpu->icount_decr.u16.high.
*/
+if (cpu->cflags_next_tb == -1
+&& (!use_icount || !(tb->cflags & CF_USE_ICOUNT)
+|| cpu_neg(cpu)->icount_decr.u16.low >= tb->icount)) {
+/*
+ * icount is disabled or there are enough instructions
+ * in the budget, do not retranslate this block with
+ * different parameters.
+ */
+cpu->cflags_next_tb = tb->cflags;
+}
return;
}
[PATCH v2 3/3] softmmu: fix watchpoints on memory used by vCPU internals
When vCPU processes interrupt request or exception, it can save
register values to the memory. Watchpoints may also be set on
these memory cells. In this case watchpoint processing code should
not retranslate the block which accessed the memory, because there
is no such block at all. "After access" watchpoint also can't be
used in such case.
This patch adds some conditions to prevent failures when watchpoint
is set on memory used for saving the registers on interrupt request.
Signed-off-by: Pavel Dovgalyuk
---
softmmu/physmem.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 314f8b439c..53edcf9a51 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -886,6 +886,14 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr
len,
assert(tcg_enabled());
if (cpu->watchpoint_hit) {
+if (!ra) {
+/*
+ * Another memory access after hitting the watchpoint.
+ * There is no translation block and interrupt request
+ * is already set.
+ */
+return;
+}
/*
* We re-entered the check after replacing the TB.
* Now raise the debug interrupt so that it will
@@ -936,6 +944,12 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr
len,
continue;
}
cpu->watchpoint_hit = wp;
+if (!ra) {
+/* We're not in the TB, can't stop before the access. */
+g_assert(!(wp->flags & BP_STOP_BEFORE_ACCESS));
+cpu_interrupt(cpu, CPU_INTERRUPT_DEBUG);
+return;
+}
mmap_lock();
/* This call also restores vCPU state */
[PATCH v2 2/3] softmmu: fix watchpoint-interrupt races
Watchpoint may be processed in two phases. First one is detecting
the instruction with target memory access. And the second one is
executing only one instruction and setting the debug interrupt flag.
Hardware interrupts can break this sequence when they happen after
the first watchpoint phase.
This patch postpones the interrupt request until watchpoint is
processed.
Signed-off-by: Pavel Dovgalyuk
---
accel/tcg/cpu-exec.c |5 +
1 file changed, 5 insertions(+)
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index df12452b8f..e4526c2f5e 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -742,6 +742,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
qemu_mutex_unlock_iothread();
return true;
}
+/* Process watchpoints first, or interrupts will ruin everything */
+if (cpu->watchpoint_hit) {
+qemu_mutex_unlock_iothread();
+return false;
+}
#if !defined(CONFIG_USER_ONLY)
if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
/* Do nothing */
Re: [PATCH 2/3] target/ppc: Implement Vector Extract Mask
On 11/10/21 7:56 PM, matheus.fe...@eldorado.org.br wrote:
From: Matheus Ferst
Implement the following PowerISA v3.1 instructions:
vextractbm: Vector Extract Byte Mask
vextracthm: Vector Extract Halfword Mask
vextractwm: Vector Extract Word Mask
vextractdm: Vector Extract Doubleword Mask
vextractqm: Vector Extract Quadword Mask
Signed-off-by: Matheus Ferst
---
target/ppc/insn32.decode| 6 ++
target/ppc/translate/vmx-impl.c.inc | 85 +
2 files changed, 91 insertions(+)
diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
index 9a28f1d266..639ac22bf0 100644
--- a/target/ppc/insn32.decode
+++ b/target/ppc/insn32.decode
@@ -419,6 +419,12 @@ VEXPANDWM 000100 . 00010 . 1100110
@VX_tb
VEXPANDDM 000100 . 00011 . 1100110@VX_tb
VEXPANDQM 000100 . 00100 . 1100110@VX_tb
+VEXTRACTBM 000100 . 01000 . 1100110@VX_tb
+VEXTRACTHM 000100 . 01001 . 1100110@VX_tb
+VEXTRACTWM 000100 . 01010 . 1100110@VX_tb
+VEXTRACTDM 000100 . 01011 . 1100110@VX_tb
+VEXTRACTQM 000100 . 01100 . 1100110@VX_tb
+
# VSX Load/Store Instructions
LXV 01 . . . 001 @DQ_TSX
diff --git a/target/ppc/translate/vmx-impl.c.inc
b/target/ppc/translate/vmx-impl.c.inc
index 58aca58f0f..c6a30614fb 100644
--- a/target/ppc/translate/vmx-impl.c.inc
+++ b/target/ppc/translate/vmx-impl.c.inc
@@ -1539,6 +1539,91 @@ static bool trans_VEXPANDQM(DisasContext *ctx, arg_VX_tb
*a)
return true;
}
+static bool do_vextractm(DisasContext *ctx, arg_VX_tb *a, unsigned vece)
+{
+const uint64_t elem_length = 8 << vece, elem_num = 15 >> vece;
+int i = elem_num;
+uint64_t bit;
+TCGv_i64 t, b, tmp, zero;
+
+REQUIRE_INSNS_FLAGS2(ctx, ISA310);
+REQUIRE_VECTOR(ctx);
+
+t = tcg_const_i64(0);
+b = tcg_temp_new_i64();
+tmp = tcg_temp_new_i64();
+zero = tcg_constant_i64(0);
+
+get_avr64(b, a->vrb, true);
+for (bit = 1ULL << 63; i > elem_num / 2; i--, bit >>= elem_length) {
+tcg_gen_shli_i64(t, t, 1);
+tcg_gen_andi_i64(tmp, b, bit);
+tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, zero);
+tcg_gen_or_i64(t, t, tmp);
+}
This is over-complicated. Shift b into the correct position, isolate the correct bit, or
it into the result.
int ele_width = 8 << vece;
int ele_count_half = 8 >> vece;
tcg_gen_movi_i64(r, 0);
for (int w = 0; w < 2; w++) {
get_avr64(v, a->vrb, w);
for (int i = 0; i < ele_count_half; ++i) {
int b_in = i * ele_width - 1;
int b_out = w * ele_count_half + i;
tcg_gen_shri_i64(t, v, b_in - b_out);
tcg_gen_andi_i64(t, t, 1 << b_out);
tcg_gen_or_i64(r, r, t);
}
}
tcg_gen_trunc_i64_tl(gpr, r);
+TRANS(VEXTRACTBM, do_vextractm, MO_8)
+TRANS(VEXTRACTHM, do_vextractm, MO_16)
+TRANS(VEXTRACTWM, do_vextractm, MO_32)
+
+static bool trans_VEXTRACTDM(DisasContext *ctx, arg_VX_tb *a)
Should be able to use the common routine above as well.
r~
Re: [PATCH 4/4] icount: preserve cflags when custom tb is about to execute
On 28.10.2021 22:26, Richard Henderson wrote:
On 10/28/21 4:48 AM, Pavel Dovgalyuk wrote:
+ if (cpu->cflags_next_tb == -1
+ && (!use_icount || !(tb->cflags & CF_USE_ICOUNT)
+ || cpu_neg(cpu)->icount_decr.u16.low >= tb->icount)) {
+ /*
+ * icount is disabled or there are enough instructions
+ * in the budget, do not retranslate this block with
+ * different parameters.
+ */
+ cpu->cflags_next_tb = tb->cflags;
+ }
I can't see that this will work.
We've been asked to exit to the main loop; probably for an interrupt.
The next thing that's likely to happen is that cpu_cc->do_interrupt will
adjust cpu state to continue in the guest interrupt handler. The
cflags_next_tb flag that you're setting up is not relevant to that context.
This seems related to Phil's reported problem
https://gitlab.com/qemu-project/qemu/-/issues/245
in which an interrupt arrives before we finish processing the watchpoint.
I *think* we need to make cflags_next_tb != -1 be treated like any other
interrupt disable bit, and delay delivery of the interrupt. Which also
means that we should not check for exit_tb at the beginning of any TB we
generate for the watchpoint step.
I simply haven't taken the time to investigate this properly.
Please check the new series, it probably solves the problem described in
that issue.
Pavel Dovgalyuk
[PATCH v2 0/3] Some watchpoint-related patches
The series includes several watchpoint-related patches. v2 changes: - added patch to fix races with interrupts - added patch to process watchpoints-on-stack - removed upstreamed patches --- Pavel Dovgalyuk (3): icount: preserve cflags when custom tb is about to execute softmmu: fix watchpoint-interrupt races softmmu: fix watchpoints on memory used by vCPU internals accel/tcg/cpu-exec.c | 5 + softmmu/physmem.c| 14 ++ 2 files changed, 19 insertions(+) -- Pavel Dovgalyuk
[PATCH v3 5/6] target/i386/sev: Perform padding calculations at compile-time
In sev_add_kernel_loader_hashes, the sizes of structs are known at
compile-time, so calculate needed padding at compile-time.
No functional change intended.
Signed-off-by: Dov Murik
Reviewed-by: Dr. David Alan Gilbert
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Daniel P. Berrangé
Acked-by: Brijesh Singh
---
target/i386/sev.c | 28 ++--
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index d11b512361..4fd258a570 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -110,9 +110,19 @@ typedef struct QEMU_PACKED SevHashTable {
SevHashTableEntry cmdline;
SevHashTableEntry initrd;
SevHashTableEntry kernel;
-uint8_t padding[];
} SevHashTable;
+/*
+ * Data encrypted by sev_encrypt_flash() must be padded to a multiple of
+ * 16 bytes.
+ */
+typedef struct QEMU_PACKED PaddedSevHashTable {
+SevHashTable ht;
+uint8_t padding[ROUND_UP(sizeof(SevHashTable), 16) - sizeof(SevHashTable)];
+} PaddedSevHashTable;
+
+QEMU_BUILD_BUG_ON(sizeof(PaddedSevHashTable) % 16 != 0);
+
static SevGuestState *sev_guest;
static Error *sev_mig_blocker;
@@ -1216,12 +1226,12 @@ bool
sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
uint8_t *data;
SevHashTableDescriptor *area;
SevHashTable *ht;
+PaddedSevHashTable *padded_ht;
uint8_t cmdline_hash[HASH_SIZE];
uint8_t initrd_hash[HASH_SIZE];
uint8_t kernel_hash[HASH_SIZE];
uint8_t *hashp;
size_t hash_len = HASH_SIZE;
-int aligned_len = ROUND_UP(sizeof(SevHashTable), 16);
/*
* Only add the kernel hashes if the sev-guest configuration explicitly
@@ -1237,7 +1247,7 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext
*ctx, Error **errp)
return false;
}
area = (SevHashTableDescriptor *)data;
-if (!area->base || area->size < aligned_len) {
+if (!area->base || area->size < sizeof(PaddedSevHashTable)) {
error_setg(errp, "SEV: guest firmware hashes table area is invalid "
"(base=0x%x size=0x%x)", area->base, area->size);
return false;
@@ -1282,7 +1292,8 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext
*ctx, Error **errp)
* Populate the hashes table in the guest's memory at the OVMF-designated
* area for the SEV hashes table
*/
-ht = qemu_map_ram_ptr(NULL, area->base);
+padded_ht = qemu_map_ram_ptr(NULL, area->base);
+ht = &padded_ht->ht;
ht->guid = sev_hash_table_header_guid;
ht->len = sizeof(*ht);
@@ -1299,13 +1310,10 @@ bool
sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
ht->kernel.len = sizeof(ht->kernel);
memcpy(ht->kernel.hash, kernel_hash, sizeof(ht->kernel.hash));
-/* When calling sev_encrypt_flash, the length has to be 16 byte aligned */
-if (aligned_len != ht->len) {
-/* zero the excess data so the measurement can be reliably calculated
*/
-memset(ht->padding, 0, aligned_len - ht->len);
-}
+/* zero the excess data so the measurement can be reliably calculated */
+memset(padded_ht->padding, 0, sizeof(padded_ht->padding));
-if (sev_encrypt_flash((uint8_t *)ht, aligned_len, errp) < 0) {
+if (sev_encrypt_flash((uint8_t *)padded_ht, sizeof(*padded_ht), errp) < 0)
{
return false;
}
--
2.25.1
[PATCH v3 3/6] target/i386/sev: Rephrase error message when no hashes table in guest firmware
Signed-off-by: Dov Murik
Acked-by: Brijesh Singh
---
target/i386/sev.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index e3abbeef68..6ff196f7ad 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1232,7 +1232,8 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext
*ctx, Error **errp)
}
if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
-error_setg(errp, "SEV: kernel specified but OVMF has no hash table
guid");
+error_setg(errp, "SEV: kernel specified but guest firmware "
+ "has no hashes table GUID");
return false;
}
area = (SevHashTableDescriptor *)data;
--
2.25.1
[PATCH v3 2/6] target/i386/sev: Add kernel hashes only if sev-guest.kernel-hashes=on
Commit cff03145ed3c ("sev/i386: Introduce sev_add_kernel_loader_hashes
for measured linux boot", 2021-09-30) introduced measured direct boot
with -kernel, using an OVMF-designated hashes table which QEMU fills.
However, if OVMF doesn't designate such an area, QEMU would completely
abort the VM launch. This breaks launching with -kernel using older
OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID.
Fix that so QEMU will only look for the hashes table if the sev-guest
kernel-hashes option is set to on. Otherwise, QEMU won't look for the
designated area in OVMF and won't fill that area.
To enable addition of kernel hashes, launch the guest with:
-object sev-guest,...,kernel-hashes=on
Signed-off-by: Dov Murik
Reported-by: Tom Lendacky
Reviewed-by: Daniel P. Berrangé
Acked-by: Brijesh Singh
---
target/i386/sev.c | 8
1 file changed, 8 insertions(+)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index cad32812f5..e3abbeef68 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1223,6 +1223,14 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext
*ctx, Error **errp)
size_t hash_len = HASH_SIZE;
int aligned_len;
+/*
+ * Only add the kernel hashes if the sev-guest configuration explicitly
+ * stated kernel-hashes=on.
+ */
+if (!sev_guest->kernel_hashes) {
+return false;
+}
+
if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
error_setg(errp, "SEV: kernel specified but OVMF has no hash table
guid");
return false;
--
2.25.1
[PATCH v3 4/6] target/i386/sev: Fail when invalid hashes table area detected
Commit cff03145ed3c ("sev/i386: Introduce sev_add_kernel_loader_hashes
for measured linux boot", 2021-09-30) introduced measured direct boot
with -kernel, using an OVMF-designated hashes table which QEMU fills.
However, no checks are performed on the validity of the hashes area
designated by OVMF. Specifically, if OVMF publishes the
SEV_HASH_TABLE_RV_GUID entry but it is filled with zeroes, this will
cause QEMU to write the hashes entries over the first page of the
guest's memory (GPA 0).
Add validity checks to the published area. If the hashes table area's
base address is zero, or its size is too small to fit the aligned hashes
table, display an error and stop the guest launch. In such case, the
following error will be displayed:
qemu-system-x86_64: SEV: guest firmware hashes table area is invalid
(base=0x0 size=0x0)
Signed-off-by: Dov Murik
Reported-by: Brijesh Singh
Reviewed-by: Daniel P. Berrangé
Acked-by: Brijesh Singh
---
target/i386/sev.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 6ff196f7ad..d11b512361 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1221,7 +1221,7 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext
*ctx, Error **errp)
uint8_t kernel_hash[HASH_SIZE];
uint8_t *hashp;
size_t hash_len = HASH_SIZE;
-int aligned_len;
+int aligned_len = ROUND_UP(sizeof(SevHashTable), 16);
/*
* Only add the kernel hashes if the sev-guest configuration explicitly
@@ -1237,6 +1237,11 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext
*ctx, Error **errp)
return false;
}
area = (SevHashTableDescriptor *)data;
+if (!area->base || area->size < aligned_len) {
+error_setg(errp, "SEV: guest firmware hashes table area is invalid "
+ "(base=0x%x size=0x%x)", area->base, area->size);
+return false;
+}
/*
* Calculate hash of kernel command-line with the terminating null byte. If
@@ -1295,7 +1300,6 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext
*ctx, Error **errp)
memcpy(ht->kernel.hash, kernel_hash, sizeof(ht->kernel.hash));
/* When calling sev_encrypt_flash, the length has to be 16 byte aligned */
-aligned_len = ROUND_UP(ht->len, 16);
if (aligned_len != ht->len) {
/* zero the excess data so the measurement can be reliably calculated
*/
memset(ht->padding, 0, aligned_len - ht->len);
--
2.25.1
[PATCH-for-6.2 v3 0/6] tests/unit: Fix test-smp-parse
Yet another approach to fix test-smp-parse. v2 from Yanan Wang: https://lore.kernel.org/qemu-devel/2021024429.10568-1-wangyana...@huawei.com/ Here we use the QOM class_init() to avoid having to deal with object_unref() and deinit(). I suggest to rename smp_parse() -> machine_parse_smp_config() after the rc0 more as a documentation change rather than an API change, since this method got added last week and doesn't follow the rest of the machine API. Supersedes: <2021024429.10568-1-wangyana...@huawei.com> Philippe Mathieu-Daudé (6): tests/unit/test-smp-parse: Restore MachineClass fields after modifying tests/unit/test-smp-parse: QOM'ify smp_machine_class_init() tests/unit/test-smp-parse: Explicit MachineClass name tests/unit/test-smp-parse: Simplify pointer to compound literal use tests/unit/test-smp-parse: Constify some pointer/struct hw/core: Rename smp_parse() -> machine_parse_smp_config() include/hw/boards.h | 3 +- hw/core/machine-smp.c | 6 +- hw/core/machine.c | 2 +- tests/unit/test-smp-parse.c | 123 +++- 4 files changed, 72 insertions(+), 62 deletions(-) -- 2.31.1
[PATCH v3 6/6] target/i386/sev: Replace qemu_map_ram_ptr with address_space_map
Use address_space_map/unmap and check for errors.
Signed-off-by: Dov Murik
Reviewed-by: Daniel P. Berrangé
Acked-by: Brijesh Singh
---
target/i386/sev.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 4fd258a570..c21f526bf2 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -37,6 +37,7 @@
#include "qapi/qmp/qerror.h"
#include "exec/confidential-guest-support.h"
#include "hw/i386/pc.h"
+#include "exec/address-spaces.h"
#define TYPE_SEV_GUEST "sev-guest"
OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
@@ -1232,6 +1233,9 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext
*ctx, Error **errp)
uint8_t kernel_hash[HASH_SIZE];
uint8_t *hashp;
size_t hash_len = HASH_SIZE;
+hwaddr mapped_len = sizeof(*padded_ht);
+MemTxAttrs attrs = { 0 };
+bool ret = true;
/*
* Only add the kernel hashes if the sev-guest configuration explicitly
@@ -1292,7 +1296,11 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext
*ctx, Error **errp)
* Populate the hashes table in the guest's memory at the OVMF-designated
* area for the SEV hashes table
*/
-padded_ht = qemu_map_ram_ptr(NULL, area->base);
+padded_ht = address_space_map(&address_space_memory, area->base,
&mapped_len, true, attrs);
+if (!padded_ht || mapped_len != sizeof(*padded_ht)) {
+error_setg(errp, "SEV: cannot map hashes table guest memory area");
+return false;
+}
ht = &padded_ht->ht;
ht->guid = sev_hash_table_header_guid;
@@ -1314,10 +1322,12 @@ bool
sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
memset(padded_ht->padding, 0, sizeof(padded_ht->padding));
if (sev_encrypt_flash((uint8_t *)padded_ht, sizeof(*padded_ht), errp) < 0)
{
-return false;
+ret = false;
}
-return true;
+address_space_unmap(&address_space_memory, padded_ht, mapped_len, true,
mapped_len);
+
+return ret;
}
static void
--
2.25.1
[PATCH-for-6.2 v3 2/6] tests/unit/test-smp-parse: QOM'ify smp_machine_class_init()
smp_machine_class_init() is the actual TypeInfo::class_init().
Declare it as such in smp_machine_info, and avoid to call it
manually in each test. Move smp_machine_info definition just
before we register the type to avoid a forward declaration.
Signed-off-by: Philippe Mathieu-Daudé
---
tests/unit/test-smp-parse.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index bd11fbe91de..51670297bf9 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -75,14 +75,6 @@ typedef struct SMPTestData {
const char *expect_error;
} SMPTestData;
-/* Type info of the tested machine */
-static const TypeInfo smp_machine_info = {
-.name = TYPE_MACHINE,
-.parent = TYPE_OBJECT,
-.class_size = sizeof(MachineClass),
-.instance_size = sizeof(MachineState),
-};
-
/*
* List all the possible valid sub-collections of the generic 5
* topology parameters (i.e. cpus/maxcpus/sockets/cores/threads),
@@ -480,9 +472,10 @@ static void unsupported_params_init(MachineClass *mc,
SMPTestData *data)
}
}
-/* Reset the related machine properties before each sub-test */
-static void smp_machine_class_init(MachineClass *mc)
+static void machine_class_init(ObjectClass *oc, void *data)
{
+MachineClass *mc = MACHINE_CLASS(oc);
+
mc->min_cpus = MIN_CPUS;
mc->max_cpus = MAX_CPUS;
@@ -498,8 +491,6 @@ static void test_generic(void)
SMPTestData *data = &(SMPTestData){{ }};
int i;
-smp_machine_class_init(mc);
-
for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
*data = data_generic_valid[i];
unsupported_params_init(mc, data);
@@ -539,7 +530,6 @@ static void test_with_dies(void)
unsigned int num_dies = 2;
int i;
-smp_machine_class_init(mc);
mc->smp_props.dies_supported = true;
for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
@@ -582,6 +572,15 @@ static void test_with_dies(void)
object_unref(obj);
}
+/* Type info of the tested machine */
+static const TypeInfo smp_machine_info = {
+.name = TYPE_MACHINE,
+.parent = TYPE_OBJECT,
+.class_init = machine_class_init,
+.class_size = sizeof(MachineClass),
+.instance_size = sizeof(MachineState),
+};
+
int main(int argc, char *argv[])
{
g_test_init(&argc, &argv, NULL);
--
2.31.1
[PATCH-for-6.2 v3 1/6] tests/unit/test-smp-parse: Restore MachineClass fields after modifying
There is a single MachineClass object, registered with type_register_static(&smp_machine_info). Since the same object is used multiple times (an MachineState object is instantiated in both test_generic and test_with_dies), we should restore its internal state after modifying for the test purpose. Signed-off-by: Philippe Mathieu-Daudé --- tests/unit/test-smp-parse.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c index cbe0c990494..bd11fbe91de 100644 --- a/tests/unit/test-smp-parse.c +++ b/tests/unit/test-smp-parse.c @@ -512,7 +512,7 @@ static void test_generic(void) smp_parse_test(ms, data, true); } -/* Reset the supported min CPUs and max CPUs */ +/* Force invalid min CPUs and max CPUs */ mc->min_cpus = 2; mc->max_cpus = 511; @@ -523,6 +523,10 @@ static void test_generic(void) smp_parse_test(ms, data, false); } +/* Reset the supported min CPUs and max CPUs */ +mc->min_cpus = MIN_CPUS; +mc->max_cpus = MAX_CPUS; + object_unref(obj); } -- 2.31.1
[PATCH v3 0/6] SEV: add kernel-hashes=on for measured -kernel launch
Tom Lendacky and Brijesh Singh reported two issues with launching SEV guests with the -kernel QEMU option when an old [1] or wrongly configured [2] OVMF images are used. To fix these issues, these series "hides" the whole kernel hashes additions behind a kernel-hashes=on option (with default value of "off"). This allows existing scenarios to work without change, and explicitly forces kernel hashes additions for guests that require that. Patch 1 introduces a new boolean option "kernel-hashes" on the sev-guest object, and patch 2 causes QEMU to add kernel hashes only if its explicitly set to "on". This will mitigate both experienced issues because the default of the new setting is off, and therefore is backward compatible with older OVMF images (which don't have a designated hashes table area) or with guests that don't wish to measure the kernel/initrd. Patch 3 fixes the wording on the error message displayed when no hashes table is found in the guest firmware. Patch 4 detects incorrect address and length of the guest firmware hashes table area and fails the boot. Patch 5 is a refactoring of parts of the same function sev_add_kernel_loader_hashes() to calculate all padding sizes at compile-time. Patch 6 also changes the same function and replaces the call to qemu_map_ram_ptr() with address_space_map() to allow for error detection. Patches 5-6 are not required to fix the issues above, but are suggested as an improvement (no functional change intended). To enable addition of kernel/initrd/cmdline hashes into the SEV guest at launch time, specify: qemu-system-x86_64 ... -object sev-guest,...,kernel-hashes=on [1] https://lore.kernel.org/qemu-devel/3b9d10d9-5d9c-da52-f18c-cd93c1931...@amd.com/ [2] https://lore.kernel.org/qemu-devel/001dd81a-282d-c307-a657-e228480d4...@amd.com/ Changes in v3: - Patch 1/6: Add "(since 6.2)" in the documentation of the kernel-hashes option (thanks Markus) - Patch 3/6: Change error string use "kernel" instead of "-kernel" (thanks Daniel) v2: https://lore.kernel.org/qemu-devel/20211108134840.2757206-1-dovmu...@linux.ibm.com/ Changes in v2: - Instead of trying to figure out whether to add hashes or not, explicity declare an option (kernel-hashes=on) for that. When that option is turned on, fail if the hashes cannot be added. - Rephrase error message when no hashes table GUID is found. - Replace qemu_map_ram_ptr with address_space_map v1: https://lore.kernel.org/qemu-devel/20211101102136.1706421-1-dovmu...@linux.ibm.com/ Dov Murik (6): qapi/qom,target/i386: sev-guest: Introduce kernel-hashes=on|off option target/i386/sev: Add kernel hashes only if sev-guest.kernel-hashes=on target/i386/sev: Rephrase error message when no hashes table in guest firmware target/i386/sev: Fail when invalid hashes table area detected target/i386/sev: Perform padding calculations at compile-time target/i386/sev: Replace qemu_map_ram_ptr with address_space_map qapi/qom.json | 7 - target/i386/sev.c | 77 +++ qemu-options.hx | 6 +++- 3 files changed, 75 insertions(+), 15 deletions(-) base-commit: af531756d25541a1b3b3d9a14e72e7fedd941a2e -- 2.25.1 Dov Murik (6): qapi/qom,target/i386: sev-guest: Introduce kernel-hashes=on|off option target/i386/sev: Add kernel hashes only if sev-guest.kernel-hashes=on target/i386/sev: Rephrase error message when no hashes table in guest firmware target/i386/sev: Fail when invalid hashes table area detected target/i386/sev: Perform padding calculations at compile-time target/i386/sev: Replace qemu_map_ram_ptr with address_space_map qapi/qom.json | 7 - target/i386/sev.c | 77 +++ qemu-options.hx | 6 +++- 3 files changed, 75 insertions(+), 15 deletions(-) base-commit: af531756d25541a1b3b3d9a14e72e7fedd941a2e -- 2.25.1
[PATCH-for-6.2 v3 4/6] tests/unit/test-smp-parse: Simplify pointer to compound literal use
We can simply use a local variable (and pass its pointer) instead
of a pointer to a compound literal.
Signed-off-by: Philippe Mathieu-Daudé
---
tests/unit/test-smp-parse.c | 64 ++---
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index de6d226b455..83a5b8ffdcf 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -492,19 +492,19 @@ static void test_generic(void)
Object *obj = object_new(TYPE_MACHINE);
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
-SMPTestData *data = &(SMPTestData){{ }};
+SMPTestData data = {};
int i;
for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
-*data = data_generic_valid[i];
-unsupported_params_init(mc, data);
+data = data_generic_valid[i];
+unsupported_params_init(mc, &data);
-smp_parse_test(ms, data, true);
+smp_parse_test(ms, &data, true);
/* Unsupported parameters can be provided with their values as 1 */
-data->config.has_dies = true;
-data->config.dies = 1;
-smp_parse_test(ms, data, true);
+data.config.has_dies = true;
+data.config.dies = 1;
+smp_parse_test(ms, &data, true);
}
/* Force invalid min CPUs and max CPUs */
@@ -512,10 +512,10 @@ static void test_generic(void)
mc->max_cpus = 511;
for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
-*data = data_generic_invalid[i];
-unsupported_params_init(mc, data);
+data = data_generic_invalid[i];
+unsupported_params_init(mc, &data);
-smp_parse_test(ms, data, false);
+smp_parse_test(ms, &data, false);
}
/* Reset the supported min CPUs and max CPUs */
@@ -530,47 +530,47 @@ static void test_with_dies(void)
Object *obj = object_new(TYPE_MACHINE);
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
-SMPTestData *data = &(SMPTestData){{ }};
+SMPTestData data = {};
unsigned int num_dies = 2;
int i;
mc->smp_props.dies_supported = true;
for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
-*data = data_generic_valid[i];
-unsupported_params_init(mc, data);
+data = data_generic_valid[i];
+unsupported_params_init(mc, &data);
/* when dies parameter is omitted, it will be set as 1 */
-data->expect_prefer_sockets.dies = 1;
-data->expect_prefer_cores.dies = 1;
+data.expect_prefer_sockets.dies = 1;
+data.expect_prefer_cores.dies = 1;
-smp_parse_test(ms, data, true);
+smp_parse_test(ms, &data, true);
/* when dies parameter is specified */
-data->config.has_dies = true;
-data->config.dies = num_dies;
-if (data->config.has_cpus) {
-data->config.cpus *= num_dies;
+data.config.has_dies = true;
+data.config.dies = num_dies;
+if (data.config.has_cpus) {
+data.config.cpus *= num_dies;
}
-if (data->config.has_maxcpus) {
-data->config.maxcpus *= num_dies;
+if (data.config.has_maxcpus) {
+data.config.maxcpus *= num_dies;
}
-data->expect_prefer_sockets.dies = num_dies;
-data->expect_prefer_sockets.cpus *= num_dies;
-data->expect_prefer_sockets.max_cpus *= num_dies;
-data->expect_prefer_cores.dies = num_dies;
-data->expect_prefer_cores.cpus *= num_dies;
-data->expect_prefer_cores.max_cpus *= num_dies;
+data.expect_prefer_sockets.dies = num_dies;
+data.expect_prefer_sockets.cpus *= num_dies;
+data.expect_prefer_sockets.max_cpus *= num_dies;
+data.expect_prefer_cores.dies = num_dies;
+data.expect_prefer_cores.cpus *= num_dies;
+data.expect_prefer_cores.max_cpus *= num_dies;
-smp_parse_test(ms, data, true);
+smp_parse_test(ms, &data, true);
}
for (i = 0; i < ARRAY_SIZE(data_with_dies_invalid); i++) {
-*data = data_with_dies_invalid[i];
-unsupported_params_init(mc, data);
+data = data_with_dies_invalid[i];
+unsupported_params_init(mc, &data);
-smp_parse_test(ms, data, false);
+smp_parse_test(ms, &data, false);
}
object_unref(obj);
--
2.31.1
[PATCH v3 1/6] qapi/qom, target/i386: sev-guest: Introduce kernel-hashes=on|off option
Introduce new boolean 'kernel-hashes' option on the sev-guest object.
It will be used to to decide whether to add the hashes of
kernel/initrd/cmdline to SEV guest memory when booting with -kernel.
The default value is 'off'.
Signed-off-by: Dov Murik
Reviewed-by: Daniel P. Berrangé
Acked-by: Brijesh Singh
---
qapi/qom.json | 7 ++-
target/i386/sev.c | 20
qemu-options.hx | 6 +-
3 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/qapi/qom.json b/qapi/qom.json
index ccd1167808..eeb5395ff3 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -769,6 +769,10 @@
# @reduced-phys-bits: number of bits in physical addresses that become
# unavailable when SEV is enabled
#
+# @kernel-hashes: if true, add hashes of kernel/initrd/cmdline to a
+# designated guest firmware page for measured boot
+# with -kernel (default: false) (since 6.2)
+#
# Since: 2.12
##
{ 'struct': 'SevGuestProperties',
@@ -778,7 +782,8 @@
'*policy': 'uint32',
'*handle': 'uint32',
'*cbitpos': 'uint32',
-'reduced-phys-bits': 'uint32' } }
+'reduced-phys-bits': 'uint32',
+'*kernel-hashes': 'bool' } }
##
# @ObjectType:
diff --git a/target/i386/sev.c b/target/i386/sev.c
index eede07f11d..cad32812f5 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -62,6 +62,7 @@ struct SevGuestState {
char *session_file;
uint32_t cbitpos;
uint32_t reduced_phys_bits;
+bool kernel_hashes;
/* runtime state */
uint32_t handle;
@@ -327,6 +328,20 @@ sev_guest_set_sev_device(Object *obj, const char *value,
Error **errp)
sev->sev_device = g_strdup(value);
}
+static bool sev_guest_get_kernel_hashes(Object *obj, Error **errp)
+{
+SevGuestState *sev = SEV_GUEST(obj);
+
+return sev->kernel_hashes;
+}
+
+static void sev_guest_set_kernel_hashes(Object *obj, bool value, Error **errp)
+{
+SevGuestState *sev = SEV_GUEST(obj);
+
+sev->kernel_hashes = value;
+}
+
static void
sev_guest_class_init(ObjectClass *oc, void *data)
{
@@ -345,6 +360,11 @@ sev_guest_class_init(ObjectClass *oc, void *data)
sev_guest_set_session_file);
object_class_property_set_description(oc, "session-file",
"guest owners session parameters (encoded with base64)");
+object_class_property_add_bool(oc, "kernel-hashes",
+ sev_guest_get_kernel_hashes,
+ sev_guest_set_kernel_hashes);
+object_class_property_set_description(oc, "kernel-hashes",
+"add kernel hashes to guest firmware for measured Linux boot");
}
static void
diff --git a/qemu-options.hx b/qemu-options.hx
index f051536b63..a11c2b29f2 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -5189,7 +5189,7 @@ SRST
-object secret,id=sec0,keyid=secmaster0,format=base64,\\
data=$SECRET,iv=$(
[PATCH-for-6.2 v3 3/6] tests/unit/test-smp-parse: Explicit MachineClass name
If the MachineClass::name pointer is not explicitly set, it is NULL.
Per the C standard, passing a NULL pointer to printf "%s" format is
undefined. Some implementations display it as 'NULL', other as 'null'.
Since we are comparing the formatted output, we need a stable value.
The easiest is to explicit a machine name string.
Signed-off-by: Philippe Mathieu-Daudé
---
tests/unit/test-smp-parse.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 51670297bf9..de6d226b455 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -23,6 +23,8 @@
#define MIN_CPUS 1 /* set the min CPUs supported by the machine as 1 */
#define MAX_CPUS 512 /* set the max CPUs supported by the machine as 512 */
+#define SMP_MACHINE_NAME "TEST-SMP"
+
/*
* Used to define the generic 3-level CPU topology hierarchy
* -sockets/cores/threads
@@ -307,13 +309,13 @@ static struct SMPTestData data_generic_invalid[] = {
* should tweak the supported min CPUs to 2 for testing */
.config = SMP_CONFIG_GENERIC(T, 1, F, 0, F, 0, F, 0, F, 0),
.expect_error = "Invalid SMP CPUs 1. The min CPUs supported "
-"by machine '(null)' is 2",
+"by machine '" SMP_MACHINE_NAME "' is 2",
}, {
/* config: -smp 512
* should tweak the supported max CPUs to 511 for testing */
.config = SMP_CONFIG_GENERIC(T, 512, F, 0, F, 0, F, 0, F, 0),
.expect_error = "Invalid SMP CPUs 512. The max CPUs supported "
-"by machine '(null)' is 511",
+"by machine '" SMP_MACHINE_NAME "' is 511",
},
};
@@ -481,6 +483,8 @@ static void machine_class_init(ObjectClass *oc, void *data)
mc->smp_props.prefer_sockets = true;
mc->smp_props.dies_supported = false;
+
+mc->name = g_strdup(SMP_MACHINE_NAME);
}
static void test_generic(void)
--
2.31.1
Re: [PATCH v2 0/6] SEV: add kernel-hashes=on for measured -kernel launch
On 11/11/2021 11:39, Daniel P. Berrangé wrote: > On Mon, Nov 08, 2021 at 01:48:34PM +, Dov Murik wrote: >> Tom Lendacky and Brijesh Singh reported two issues with launching SEV >> guests with the -kernel QEMU option when an old [1] or wrongly configured [2] >> OVMF images are used. >> >> To fix these issues, these series "hides" the whole kernel hashes >> additions behind a kernel-hashes=on option (with default value of >> "off"). This allows existing scenarios to work without change, and >> explicitly forces kernel hashes additions for guests that require that. > > We need to to get this into 6.2 to adress the regression vs previous > QEMU releases. > > There's just a couple of small review points. If you can repost > with the easy fixes, then we can get this queued. > Sent v3 now. Patch 3/6 (error message rephrase) still misses Reviewed-by. Thanks, -Dov
[PATCH-for-6.2 v3 6/6] hw/core: Rename smp_parse() -> machine_parse_smp_config()
All methods related to MachineState are prefixed with "machine_".
smp_parse() does not need to be an exception. Rename it and
const'ify the SMPConfiguration argument, since it doesn't need
to be modified.
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/boards.h | 3 ++-
hw/core/machine-smp.c | 6 --
hw/core/machine.c | 2 +-
tests/unit/test-smp-parse.c | 8
4 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 9c1c1901046..7597cec4400 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -34,7 +34,8 @@ HotpluggableCPUList
*machine_query_hotpluggable_cpus(MachineState *machine);
void machine_set_cpu_numa_node(MachineState *machine,
const CpuInstanceProperties *props,
Error **errp);
-void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp);
+void machine_parse_smp_config(MachineState *ms,
+ const SMPConfiguration *config, Error **errp);
/**
* machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 116a0cbbfab..2cbfd574293 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -44,7 +44,8 @@ static char *cpu_hierarchy_to_string(MachineState *ms)
}
/*
- * smp_parse - Generic function used to parse the given SMP configuration
+ * machine_parse_smp_config: Generic function used to parse the given
+ * SMP configuration
*
* Any missing parameter in "cpus/maxcpus/sockets/cores/threads" will be
* automatically computed based on the provided ones.
@@ -63,7 +64,8 @@ static char *cpu_hierarchy_to_string(MachineState *ms)
* introduced topology members which are likely to be target specific should
* be directly set as 1 if they are omitted (e.g. dies for PC since 4.1).
*/
-void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
+void machine_parse_smp_config(MachineState *ms,
+ const SMPConfiguration *config, Error **errp)
{
MachineClass *mc = MACHINE_GET_CLASS(ms);
unsigned cpus= config->has_cpus ? config->cpus : 0;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 26ec54e7261..a2d3c9969d9 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -760,7 +760,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const
char *name,
return;
}
-smp_parse(ms, config, errp);
+machine_parse_smp_config(ms, config, errp);
}
static void machine_class_init(ObjectClass *oc, void *data)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 11109752799..b158ebb16b1 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -337,7 +337,7 @@ static const struct SMPTestData data_with_dies_invalid[] = {
},
};
-static char *smp_config_to_string(SMPConfiguration *config)
+static char *smp_config_to_string(const SMPConfiguration *config)
{
return g_strdup_printf(
"(SMPConfiguration) {\n"
@@ -371,7 +371,7 @@ static char *cpu_topology_to_string(const CpuTopology *topo)
topo->cores, topo->threads, topo->max_cpus);
}
-static void check_parse(MachineState *ms, SMPConfiguration *config,
+static void check_parse(MachineState *ms, const SMPConfiguration *config,
const CpuTopology *expect_topo, const char *expect_err,
bool is_valid)
{
@@ -380,8 +380,8 @@ static void check_parse(MachineState *ms, SMPConfiguration
*config,
g_autofree char *output_topo_str = NULL;
Error *err = NULL;
-/* call the generic parser smp_parse() */
-smp_parse(ms, config, &err);
+/* call the generic parser */
+machine_parse_smp_config(ms, config, &err);
output_topo_str = cpu_topology_to_string(&ms->smp);
--
2.31.1
[PATCH-for-6.2 v3 5/6] tests/unit/test-smp-parse: Constify some pointer/struct
Declare structures const when we don't need to modify
them at runtime.
Signed-off-by: Philippe Mathieu-Daudé
---
tests/unit/test-smp-parse.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 83a5b8ffdcf..11109752799 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -83,7 +83,7 @@ typedef struct SMPTestData {
* then test the automatic calculation algorithm of the missing
* values in the parser.
*/
-static struct SMPTestData data_generic_valid[] = {
+static const struct SMPTestData data_generic_valid[] = {
{
/* config: no configuration provided
* expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */
@@ -285,7 +285,7 @@ static struct SMPTestData data_generic_valid[] = {
},
};
-static struct SMPTestData data_generic_invalid[] = {
+static const struct SMPTestData data_generic_invalid[] = {
{
/* config: -smp 2,dies=2 */
.config = SMP_CONFIG_WITH_DIES(T, 2, F, 0, T, 2, F, 0, F, 0, F, 0),
@@ -319,7 +319,7 @@ static struct SMPTestData data_generic_invalid[] = {
},
};
-static struct SMPTestData data_with_dies_invalid[] = {
+static const struct SMPTestData data_with_dies_invalid[] = {
{
/* config: -smp 16,sockets=2,dies=2,cores=4,threads=2,maxcpus=16 */
.config = SMP_CONFIG_WITH_DIES(T, 16, T, 2, T, 2, T, 4, T, 2, T, 16),
@@ -356,7 +356,7 @@ static char *smp_config_to_string(SMPConfiguration *config)
config->has_maxcpus ? "true" : "false", config->maxcpus);
}
-static char *cpu_topology_to_string(CpuTopology *topo)
+static char *cpu_topology_to_string(const CpuTopology *topo)
{
return g_strdup_printf(
"(CpuTopology) {\n"
@@ -372,7 +372,7 @@ static char *cpu_topology_to_string(CpuTopology *topo)
}
static void check_parse(MachineState *ms, SMPConfiguration *config,
-CpuTopology *expect_topo, const char *expect_err,
+const CpuTopology *expect_topo, const char *expect_err,
bool is_valid)
{
g_autofree char *config_str = smp_config_to_string(config);
@@ -466,7 +466,7 @@ static void smp_parse_test(MachineState *ms, SMPTestData
*data, bool is_valid)
}
/* The parsed results of the unsupported parameters should be 1 */
-static void unsupported_params_init(MachineClass *mc, SMPTestData *data)
+static void unsupported_params_init(const MachineClass *mc, SMPTestData *data)
{
if (!mc->smp_props.dies_supported) {
data->expect_prefer_sockets.dies = 1;
--
2.31.1
Re: [PATCH v9 5/8] qmp: decode feature & status bits in virtio-status
On 11/10/21 08:49, Markus Armbruster wrote:
Jonah Palmer writes:
From: Laurent Vivier
Display feature names instead of bitmaps for host, guest, and
backend for VirtIODevice.
Display status names instead of bitmaps for VirtIODevice.
Display feature names instead of bitmaps for backend, protocol,
acked, and features (hdev->features) for vhost devices.
Decode features according to device type. Decode status
according to configuration status bitmap (config_status_map).
Decode vhost user protocol features according to vhost user
protocol bitmap (vhost_user_protocol_map).
Transport features are on the first line. Undecoded bits
(if any) are stored in a separate field. Vhost device field
wont show if there's no vhost active for a given VirtIODevice.
Signed-off-by: Jonah Palmer
[...]
diff --git a/qapi/virtio.json b/qapi/virtio.json
index 54212f2..6b11d52 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -67,6 +67,466 @@
}
##
+# @VirtioType:
+#
+# An enumeration of Virtio device types (or names)
+#
+# Since: 6.3
+##
+
+{ 'enum': 'VirtioType',
+ 'data': [ 'virtio-net', 'virtio-blk', 'virtio-serial', 'virtio-rng',
+'virtio-balloon', 'virtio-iomem', 'virtio-rpmsg',
+'virtio-scsi', 'virtio-9p', 'virtio-mac-wlan',
+'virtio-rproc-serial', 'virtio-caif', 'virtio-mem-balloon',
+'virtio-gpu', 'virtio-clk', 'virtio-input', 'vhost-vsock',
+'virtio-crypto', 'virtio-signal', 'virtio-pstore',
+'virtio-iommu', 'virtio-mem', 'virtio-sound', 'vhost-user-fs',
+'virtio-pmem', 'virtio-mac-hwsim', 'vhost-user-i2c',
+'virtio-bluetooth' ]
+}
+
+##
+# @VirtioConfigStatus:
+#
+# An enumeration of Virtio device configuration statuses
+#
+# Since: 6.3
+##
+
+{ 'enum': 'VirtioConfigStatus',
+ 'data': [ 'driver-ok', 'features-ok', 'driver', 'needs-reset',
+'failed', 'acknowledge' ]
+}
+
+##
+# @VirtioDeviceStatus:
+#
+# A structure defined to list the configuration statuses of a virtio
+# device
+#
+# @dev-status: List of decoded configuration statuses of the virtio
+# device
+#
+# @unknown-statuses: virtio device statuses bitmap that have not been decoded
Why is @dev-status singular, and @unknown-statuses plural?
I'm guessing that when I wrote it I used singular here since it was one list of
statuses, but the representation here does feel off. Maybe @statuses &
@unknown-statuses
would be a better choice?
+#
+# Since: 6.3
+##
+
+{ 'struct': 'VirtioDeviceStatus',
+ 'data': { 'dev-status': [ 'VirtioConfigStatus' ],
+'*unknown-statuses': 'uint8' } }
+
+##
+# @VhostProtocolFeature:
+#
+# An enumeration of Vhost User protocol features
+#
+# Since: 6.3
+##
+
+{ 'enum': 'VhostProtocolFeature',
+ 'data': [ 'mq', 'log-shmfd', 'rarp', 'reply-ack', 'net-mtu',
+'slave-req', 'cross-endian', 'crypto-session', 'pagefault',
+'config', 'slave-send-fd', 'host-notifier',
+'inflight-shmfd', 'reset-device', 'inband-notifications',
+'configure-mem-slots' ]
+}
+
+##
+# @VhostDeviceProtocols:
+#
+# A structure defined to list the vhost user protocol features of a
+# Vhost User device
+#
+# @features: List of decoded vhost user protocol features of a vhost
+#user device
+#
+# @unknown-protocols: vhost user device protocol features bitmap that
+# have not been decoded
Why are the known protocol features called @features, and the unknown
ones @unknown-protocols?
I agree that this is inconsistent. Maybe @protocols & @unknown-protocols
would be a better choice here as well?
+#
+# Since: 6.3
+##
+
+{ 'struct': 'VhostDeviceProtocols',
+ 'data': { 'features': [ 'VhostProtocolFeature' ],
+'*unknown-protocols': 'uint64' } }
+
+##
+# @VirtioTransportFeature:
+#
+# An enumeration of Virtio device transport features, including virtio-ring
+#
+# Since: 6.3
+##
+
+{ 'enum': 'VirtioTransportFeature',
+ 'data': [ 'notify-on-empty', 'any-layout', 'protocol-features',
+'version-1', 'iommu-platform', 'ring-packed', 'order-platform',
+'sr-iov', 'indirect-desc', 'event-idx' ]
+}
+
+##
+# @VirtioMemFeature:
+#
+# An enumeration of Virtio mem features
+#
+# Since: 6.3
+##
+
+{ 'enum': 'VirtioMemFeature',
+ 'data': [ 'acpi-pxm' ]
+}
+
+##
+# @VirtioSerialFeature:
+#
+# An enumeration of Virtio serial/console features
+#
+# Since: 6.3
+##
+
+{ 'enum': 'VirtioSerialFeature',
+ 'data': [ 'size', 'multiport', 'emerg-write' ]
+}
+
+##
+# @VirtioBlkFeature:
+#
+# An enumeration of Virtio block features
+#
+# Since: 6.3
+##
+
+{ 'enum': 'VirtioBlkFeature',
+ 'data': [ 'size-max', 'seg-max', 'geometry', 'ro', 'blk-size',
+'topology', 'mq', 'discard', 'write-zeroes', 'barrier',
+'scsi', 'flush', 'config-wce', 'log-all' ]
+}
+
+##
+# @VirtioGpuFeature:
+#
+# An enumeration of Virtio gpu features
+#
+# Since: 6.3
+##
+
+{ 'enum': 'VirtioGpuFeature',
+ 'data': [ 'virgl', 'edid', 'r
Re: [PATCH v9 7/8] qmp: add QMP command x-query-virtio-queue-element
On 11/10/21 08:52, Markus Armbruster wrote:
Jonah Palmer writes:
From: Laurent Vivier
This new command shows the information of a VirtQueue element.
Signed-off-by: Jonah Palmer
[...]
diff --git a/qapi/virtio.json b/qapi/virtio.json
index 0f65044..c57fbc5 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -1061,3 +1061,180 @@
{ 'command': 'x-query-virtio-vhost-queue-status',
'data': { 'path': 'str', 'queue': 'uint16' },
'returns': 'VirtVhostQueueStatus', 'features': [ 'unstable' ] }
+
+##
+# @VirtioRingDescFlags:
+#
+# An enumeration of the virtio ring descriptor flags
+#
+# Since: 6.3
+#
+##
+
+{ 'enum': 'VirtioRingDescFlags',
+ 'data': [ 'next', 'write', 'indirect', 'avail', 'used' ]
+}
+
+##
+# @VirtioRingDesc:
+#
+# Information regarding the VRing descriptor area
+#
+# @addr: guest physical address of the descriptor data
+#
+# @len: length of the descriptor data
+#
+# @flags: list of descriptor flags
+#
+# Since: 6.3
+#
+##
+
+{ 'struct': 'VirtioRingDesc',
+ 'data': { 'addr': 'uint64',
+'len': 'uint32',
+'flags': [ 'VirtioRingDescFlags' ] } }
+
+##
+# @VirtioRingAvail:
+#
+# Information regarding the avail VRing (also known as the driver
+# area)
+#
+# @flags: VRingAvail flags
+#
+# @idx: VRingAvail index
+#
+# @ring: VRingAvail ring[] entry at provided index
+#
+# Since: 6.3
+#
+##
+
+{ 'struct': 'VirtioRingAvail',
+ 'data': { 'flags': 'uint16',
+'idx': 'uint16',
+'ring': 'uint16' } }
+
+##
+# @VirtioRingUsed:
+#
+# Information regarding the used VRing (also known as the device
+# area)
+#
+# @flags: VRingUsed flags
+#
+# @idx: VRingUsed index
+#
+# Since: 6.3
+#
+##
+
+{ 'struct': 'VirtioRingUsed',
+ 'data': { 'flags': 'uint16',
+'idx': 'uint16' } }
+
+##
+# @VirtioQueueElement:
+#
+# Information regarding a VirtQueue VirtQueueElement including
+# descriptor, driver, and device areas
+#
+# @device-name: name of the VirtIODevice which this VirtQueue belongs
+# to (for reference)
+#
+# @index: index of the element in the queue
+#
+# @ndescs: number of descriptors
+#
+# @descs: list of the descriptors
Can @ndescs ever be not equal to the length of @descs?
If no, it's redundant.
I don't believe so, no. Should I just remove @ndescs then?
Jonah
+#
+# @avail: VRingAvail info
+#
+# @used: VRingUsed info
+#
+# Since: 6.3
+#
+##
+
+{ 'struct': 'VirtioQueueElement',
+ 'data': { 'device-name': 'str',
+'index': 'uint32',
+'ndescs': 'uint32',
+'descs': [ 'VirtioRingDesc' ],
+'avail': 'VirtioRingAvail',
+'used': 'VirtioRingUsed' } }
+
+##
+# @x-query-virtio-queue-element:
+#
+# Return the information about a VirtQueue VirtQueueElement (by
+# default looks at the head of the queue)
+#
+# @path: VirtIODevice canonical QOM path
+#
+# @queue: VirtQueue index to examine
+#
+# @index: the index in the queue, by default head
+#
+# Features:
+# @unstable: This command is meant for debugging.
+#
+# Returns: VirtioQueueElement information
+#
+# Since: 6.3
+#
+# Examples:
+#
+# 1. Introspect on virtio-net virtqueue 0 at index 5
+#
+# -> { "execute": "x-query-virtio-queue-element",
+# "arguments": { "path":
"/machine/peripheral-anon/device[1]/virtio-backend",
+# "queue": 0,
+# "index": 5 }
+#}
+# <- { "return": {
+# "index": 5,
+# "ndescs": 1,
+# "device-name": "virtio-net",
+# "descs": [ { "flags": ["write"], "len": 1536, "addr": 5257305600 } ],
+# "avail": { "idx": 256, "flags": 0, "ring": 5 },
+# "used": { "idx": 13, "flags": 0 } }
+#}
+#
+# 2. Introspect on virtio-crypto virtqueue 1 at head
+#
+# -> { "execute": "x-query-virtio-queue-element",
+# "arguments": { "path": "/machine/peripheral/crypto0/virtio-backend",
+# "queue": 1 }
+#}
+# <- { "return": {
+# "index": 0,
+# "ndescs": 1,
+# "device-name": "virtio-crypto",
+# "descs": [ { "flags": [], "len": 0, "addr": 8080268923184214134 } ],
+# "avail": { "idx": 280, "flags": 0, "ring": 0 },
+# "used": { "idx": 280, "flags": 0 } }
+#}
+#
+# 3. Introspect on virtio-scsi virtqueue 2 at head
+#
+# -> { "execute": "x-query-virtio-queue-element",
+# "arguments": { "path":
"/machine/peripheral-anon/device[2]/virtio-backend",
+# "queue": 2 }
+#}
+# <- { "return": {
+# "index": 19,
+# "ndescs": 1,
+# "device-name": "virtio-scsi",
+# "descs": [ { "flags": ["used", "indirect", "write"], "len":
4099327944,
+# "addr": 12055409292258155293 } ],
+# "avail": { "idx": 1147, "flags": 0, "ring": 19 },
+# "used": { "idx": 1147, "flags": 0 } }
+#}
+#
+##
+
+{ 'command': 'x-query-virtio-queue-element',
+ 'data': { 'path': 'str', 'queue': 'uint16', '*index': 'uint16' },
+ 'returns': 'VirtioQueueElement', 'features': [ 'unstable' ] }
Re: [PATCH v3 3/6] target/i386/sev: Rephrase error message when no hashes table in guest firmware
On Thu, Nov 11, 2021 at 10:00:45AM +, Dov Murik wrote: > Signed-off-by: Dov Murik > Acked-by: Brijesh Singh > --- > target/i386/sev.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v9 8/8] hmp: add virtio commands
On 11/10/21 08:30, Markus Armbruster wrote:
Jonah Palmer writes:
From: Laurent Vivier
This patch implements the HMP versions of the virtio QMP commands.
Signed-off-by: Jonah Palmer
---
hmp-commands-info.hx | 218 ++
include/monitor/hmp.h | 5 +
monitor/hmp-cmds.c| 358 ++
3 files changed, 581 insertions(+)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 407a1da..6bf7359 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -877,3 +877,221 @@ SRST
``info sgx``
Show intel SGX information.
ERST
+
+{
+.name = "virtio",
+.args_type = "",
+.params= "",
+.help = "List all available virtio devices",
+.cmd = hmp_virtio_query,
+.flags = "p",
+},
+
+SRST
+ ``info virtio``
+List all available virtio devices
+
+Example:
+
+List all available virtio devices in the machine::
+
+(qemu) info virtio
+/machine/peripheral/vsock0/virtio-backend [vhost-vsock]
I get
docs/../hmp-commands-info.hx:899:Inconsistent literal block quoting.
This is from Sphinx. I can't see what's wrong.
I'll (hopefully) get this cleared up in the next series. Will run Sphinx
myself to double check.
Jonah
+/machine/peripheral/crypto0/virtio-backend [virtio-crypto]
+/machine/peripheral-anon/device[2]/virtio-backend [virtio-scsi]
+/machine/peripheral-anon/device[1]/virtio-backend [virtio-net]
+/machine/peripheral-anon/device[0]/virtio-backend [virtio-serial]
+
+ERST
[...]
Re: [PATCH v2 1/1] ppc/mmu_helper.c: do not truncate 'ea' in booke206_invalidate_ea_tlb()
On 11/10/21 21:25, Daniel Henrique Barboza wrote:
'tlbivax' is implemented by gen_tlbivax_booke206() via
gen_helper_booke206_tlbivax(). In case the TLB needs to be flushed,
booke206_invalidate_ea_tlb() is called. All these functions, but
booke206_invalidate_ea_tlb(), uses a 64-bit effective address 'ea'.
booke206_invalidate_ea_tlb() uses an uint32_t 'ea' argument that
truncates the original 'ea' value for apparently no particular reason.
This function retrieves the tlb pointer by calling booke206_get_tlbm(),
which also uses a target_ulong address as parameter - in this case, a
truncated 'ea' address. All the surrounding logic considers the
effective TLB address as a 64 bit value, aside from the signature of
booke206_invalidate_ea_tlb().
Last but not the least, PowerISA 2.07B section 6.11.4.9 [2] makes it
clear that the effective address "EA" is a 64 bit value.
Commit 01662f3e5133 introduced this code and no changes were made ever
since. An user detected a problem with tlbivax [1] stating that this
address truncation was the cause. This same behavior might be the source
of several subtle bugs that were never caught.
For all these reasons, this patch assumes that this address truncation
is the result of a mistake/oversight of the original commit, and changes
booke206_invalidate_ea_tlb() 'ea' argument to 'vaddr'.
[1] https://gitlab.com/qemu-project/qemu/-/issues/52
[2] https://wiki.raptorcs.com/wiki/File:PowerISA_V2.07B.pdf
Fixes: 01662f3e5133 ("PPC: Implement e500 (FSL) MMU")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/52
Signed-off-by: Daniel Henrique Barboza
---
target/ppc/mmu_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
It looks correct. I am not sure why we introduced PPC_TLBIVAX since
we don't use it for the instruction. But e200 needs it AFAICT.
Any how,
Queued for 6.2
Thanks,
C.
diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index 2cb98c5169..e0c4950dda 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -1216,7 +1216,7 @@ void helper_booke206_tlbsx(CPUPPCState *env, target_ulong
address)
}
static inline void booke206_invalidate_ea_tlb(CPUPPCState *env, int tlbn,
- uint32_t ea)
+ vaddr ea)
{
int i;
int ways = booke206_tlb_ways(env, tlbn);
Re: [PATCH] Fix tcg_out_vec_op argument type
On 27/10/2021 10.56, Miroslav Rezanina wrote:
Newly defined tcg_out_vec_op (34ef767609 tcg/s390x: Add host vector framework)
for s390x uses pointer argument definition.
This fails on gcc 11 as original declaration uses array argument:
In file included from ../tcg/tcg.c:430:
/builddir/build/BUILD/qemu-6.1.50/tcg/s390x/tcg-target.c.inc:2702:42: error:
argument 5 of type 'const TCGArg *' {aka 'const long unsigned int *'} declared
as a pointer [-Werror=array-parameter=]
2702 |const TCGArg *args, const int *const_args)
|~~^~~~
../tcg/tcg.c:121:41: note: previously declared as an array 'const TCGArg[16]'
{aka 'const long unsigned int[16]'}
121 |const TCGArg args[TCG_MAX_OP_ARGS],
|~^
In file included from ../tcg/tcg.c:430:
/builddir/build/BUILD/qemu-6.1.50/tcg/s390x/tcg-target.c.inc:2702:59: error:
argument 6 of type 'const int *' declared as a pointer
[-Werror=array-parameter=]
2702 |const TCGArg *args, const int *const_args)
|~~~^~
../tcg/tcg.c:122:38: note: previously declared as an array 'const int[16]'
122 |const int const_args[TCG_MAX_OP_ARGS]);
|~~^~~
Fixing argument type to pass build.
Signed-off-by: Miroslav Rezanina
---
tcg/s390x/tcg-target.c.inc | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tcg/s390x/tcg-target.c.inc b/tcg/s390x/tcg-target.c.inc
index 8938c446c8..57e803e339 100644
--- a/tcg/s390x/tcg-target.c.inc
+++ b/tcg/s390x/tcg-target.c.inc
@@ -2699,7 +2699,8 @@ static void tcg_out_dupi_vec(TCGContext *s, TCGType type,
unsigned vece,
static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
unsigned vecl, unsigned vece,
- const TCGArg *args, const int *const_args)
+ const TCGArg args[TCG_MAX_OP_ARGS],
+ const int const_args[TCG_MAX_OP_ARGS])
{
TCGType type = vecl + TCG_TYPE_V64;
TCGArg a0 = args[0], a1 = args[1], a2 = args[2];
Reviewed-by: Thomas Huth
Richard, do you still have a pull request planned for rc1 ? If not, I could
also take it through the s390x tree if you like.
Thomas
Re: [PATCH 3/3] target/ppc: Implement Vector Mask Move insns
On 11/10/21 7:56 PM, matheus.fe...@eldorado.org.br wrote:
+static bool do_mtvsrm(DisasContext *ctx, arg_VX_tb *a, unsigned vece)
+{
+const uint64_t elem_length = 8 << vece, highest_bit = 15 >> vece;
+int i;
+TCGv_i64 t0, t1, zero, ones;
+
+REQUIRE_INSNS_FLAGS2(ctx, ISA310);
+REQUIRE_VECTOR(ctx);
+
+t0 = tcg_const_i64(0);
+t1 = tcg_temp_new_i64();
+zero = tcg_constant_i64(0);
+ones = tcg_constant_i64(MAKE_64BIT_MASK(0, elem_length));
+
+for (i = 1 << highest_bit; i > 1 << (highest_bit / 2); i >>= 1) {
+tcg_gen_shli_i64(t0, t0, elem_length);
+tcg_gen_ext_tl_i64(t1, cpu_gpr[a->vrb]);
+tcg_gen_andi_i64(t1, t1, i);
+tcg_gen_movcond_i64(TCG_COND_NE, t1, t1, zero, ones, zero);
+tcg_gen_or_i64(t0, t0, t1);
+}
We can do better than that.
tcg_gen_extu_tl_i64(t0, gpr);
tcg_gen_extract_i64(t1, t0, elem_count_half, elem_count_half);
tcg_gen_extract_i64(t0, t0, 0, elem_count_half);
/*
* Spread the bits into their respective elements.
* E.g. for bytes:
* abcdefgh
* << 32-4
* abcdefgh
* |
* abcdefghabcdefgh
* << 16-2
* 00abcdefghabcdefgh00
* |
* 00abcdefgh00abcdefgh00abcdefgh00abcdefgh
* << 8-1
* 000abcdefgh00abcdefgh00abcdefgh00abcdefgh000
* |
* 000abcdefgXbcdefgXbcdefgXbcdefgXbcdefgXbcdefgXbcdefgXbcdefgh
* & dup(1)
* 000a000b000c000d000e000f000g000h
* * 0xff
*
*/
for (i = elem_count_half, j = 32; i > 0; i >>= 1, j >>= 1) {
tcg_gen_shli_i64(s0, t0, j - i);
tcg_gen_shli_i64(s1, t1, j - i);
tcg_gen_or_i64(t0, t0, s0);
tcg_gen_or_i64(t1, t1, s1);
}
c = dup_const(vece, 1);
tcg_gen_andi_i64(t0, t0, c);
tcg_gen_andi_i64(t1, t1, c);
c = MAKE_64BIT_MASK(0, elem_length);
tcg_gen_muli_i64(t0, t0, c);
tcg_gen_muli_i64(t1, t1, c);
set_avr64(a->vrt, t0, false);
set_avr64(a->vrt, t1, true);
r~
Re: [PATCH] Fix tcg_out_vec_op argument type
On 11.11.21 11:42, Thomas Huth wrote:
> On 27/10/2021 10.56, Miroslav Rezanina wrote:
>> Newly defined tcg_out_vec_op (34ef767609 tcg/s390x: Add host vector
>> framework)
>> for s390x uses pointer argument definition.
>> This fails on gcc 11 as original declaration uses array argument:
>>
>> In file included from ../tcg/tcg.c:430:
>> /builddir/build/BUILD/qemu-6.1.50/tcg/s390x/tcg-target.c.inc:2702:42: error:
>> argument 5 of type 'const TCGArg *' {aka 'const long unsigned int *'}
>> declared as a pointer [-Werror=array-parameter=]
>> 2702 |const TCGArg *args, const int
>> *const_args)
>>|~~^~~~
>> ../tcg/tcg.c:121:41: note: previously declared as an array 'const
>> TCGArg[16]' {aka 'const long unsigned int[16]'}
>>121 |const TCGArg args[TCG_MAX_OP_ARGS],
>>|~^
>> In file included from ../tcg/tcg.c:430:
>> /builddir/build/BUILD/qemu-6.1.50/tcg/s390x/tcg-target.c.inc:2702:59: error:
>> argument 6 of type 'const int *' declared as a pointer
>> [-Werror=array-parameter=]
>> 2702 |const TCGArg *args, const int
>> *const_args)
>>|~~~^~
>> ../tcg/tcg.c:122:38: note: previously declared as an array 'const int[16]'
>>122 |const int const_args[TCG_MAX_OP_ARGS]);
>>|~~^~~
>>
>> Fixing argument type to pass build.
>>
>> Signed-off-by: Miroslav Rezanina
>> ---
>> tcg/s390x/tcg-target.c.inc | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tcg/s390x/tcg-target.c.inc b/tcg/s390x/tcg-target.c.inc
>> index 8938c446c8..57e803e339 100644
>> --- a/tcg/s390x/tcg-target.c.inc
>> +++ b/tcg/s390x/tcg-target.c.inc
>> @@ -2699,7 +2699,8 @@ static void tcg_out_dupi_vec(TCGContext *s, TCGType
>> type, unsigned vece,
>>
>> static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
>> unsigned vecl, unsigned vece,
>> - const TCGArg *args, const int *const_args)
>> + const TCGArg args[TCG_MAX_OP_ARGS],
>> + const int const_args[TCG_MAX_OP_ARGS])
>> {
>> TCGType type = vecl + TCG_TYPE_V64;
>> TCGArg a0 = args[0], a1 = args[1], a2 = args[2];
>
> Reviewed-by: Thomas Huth
>
Acked-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH v2 0/3] Some watchpoint-related patches
On 11.11.21 10:55, Pavel Dovgalyuk wrote: > The series includes several watchpoint-related patches. > > v2 changes: > - added patch to fix races with interrupts > - added patch to process watchpoints-on-stack > - removed upstreamed patches Out of interest, do we have any reproducer / tests for this? -- Thanks, David / dhildenb
Re: [PATCH v2 0/3] Some watchpoint-related patches
On 11.11.2021 13:48, David Hildenbrand wrote: On 11.11.21 10:55, Pavel Dovgalyuk wrote: The series includes several watchpoint-related patches. v2 changes: - added patch to fix races with interrupts - added patch to process watchpoints-on-stack - removed upstreamed patches Out of interest, do we have any reproducer / tests for this? I guess no.
Re: [PATCH] Fix tcg_out_vec_op argument type
On 11/11/21 11:46 AM, David Hildenbrand wrote:
On 11.11.21 11:42, Thomas Huth wrote:
On 27/10/2021 10.56, Miroslav Rezanina wrote:
Newly defined tcg_out_vec_op (34ef767609 tcg/s390x: Add host vector framework)
for s390x uses pointer argument definition.
This fails on gcc 11 as original declaration uses array argument:
In file included from ../tcg/tcg.c:430:
/builddir/build/BUILD/qemu-6.1.50/tcg/s390x/tcg-target.c.inc:2702:42: error:
argument 5 of type 'const TCGArg *' {aka 'const long unsigned int *'} declared
as a pointer [-Werror=array-parameter=]
2702 |const TCGArg *args, const int *const_args)
|~~^~~~
../tcg/tcg.c:121:41: note: previously declared as an array 'const TCGArg[16]'
{aka 'const long unsigned int[16]'}
121 |const TCGArg args[TCG_MAX_OP_ARGS],
|~^
In file included from ../tcg/tcg.c:430:
/builddir/build/BUILD/qemu-6.1.50/tcg/s390x/tcg-target.c.inc:2702:59: error:
argument 6 of type 'const int *' declared as a pointer
[-Werror=array-parameter=]
2702 |const TCGArg *args, const int *const_args)
|~~~^~
../tcg/tcg.c:122:38: note: previously declared as an array 'const int[16]'
122 |const int const_args[TCG_MAX_OP_ARGS]);
|~~^~~
Fixing argument type to pass build.
Signed-off-by: Miroslav Rezanina
---
tcg/s390x/tcg-target.c.inc | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tcg/s390x/tcg-target.c.inc b/tcg/s390x/tcg-target.c.inc
index 8938c446c8..57e803e339 100644
--- a/tcg/s390x/tcg-target.c.inc
+++ b/tcg/s390x/tcg-target.c.inc
@@ -2699,7 +2699,8 @@ static void tcg_out_dupi_vec(TCGContext *s, TCGType type,
unsigned vece,
static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
unsigned vecl, unsigned vece,
- const TCGArg *args, const int *const_args)
+ const TCGArg args[TCG_MAX_OP_ARGS],
+ const int const_args[TCG_MAX_OP_ARGS])
{
TCGType type = vecl + TCG_TYPE_V64;
TCGArg a0 = args[0], a1 = args[1], a2 = args[2];
Reviewed-by: Thomas Huth
Acked-by: David Hildenbrand
Queued to tcg-next, thanks.
r~
Re: [PULL 0/5] x86, RCU/TCG patches for QEMU 6.2-rc1
On 11/11/21 8:36 AM, Paolo Bonzini wrote: The following changes since commit 114f3c8cc427333dbae331dfd2ecae64676b087e: Merge remote-tracking branch 'remotes/philmd/tags/avocado-20211108' into staging (2021-11-08 18:50:09 +0100) are available in the Git repository at: https://gitlab.com/bonzini/qemu.git tags/for-upstream for you to fetch changes up to 2c3132279b9a962c27adaea53b4c8e8480385706: sgx: Reset the vEPC regions during VM reboot (2021-11-10 22:57:40 +0100) * Fixes for SGX * force_rcu notifiers Greg Kurz (2): rcu: Introduce force_rcu notifier accel/tcg: Register a force_rcu notifier Paolo Bonzini (2): target/i386: sgx: mark device not user creatable numa: avoid crash with SGX and "info numa" Yang Zhong (1): sgx: Reset the vEPC regions during VM reboot accel/tcg/tcg-accel-ops-mttcg.c | 26 + accel/tcg/tcg-accel-ops-rr.c| 10 + hw/core/numa.c | 7 ++ hw/i386/sgx-epc.c | 1 + hw/i386/sgx.c | 50 + include/qemu/rcu.h | 15 + util/rcu.c | 19 7 files changed, 128 insertions(+) Applied, thanks. r~
[PULL 0/4] Linux user for 6.2 patches
The following changes since commit 856f9fa9a2c528dc29693d3b3a64a9b93bf866a2: Merge tag 'pull-jobs-2021-11-09' of https://src.openvz.org/scm/~vsementsov/qemu into staging (2021-11-09 21:40:05 +0100) are available in the Git repository at: git://github.com/vivier/qemu.git tags/linux-user-for-6.2-pull-request for you to fetch changes up to 3c499fa24521527e933e171a9b04a896a1c29cf9: linux-user: Rewrite do_getdents, do_getdents64 (2021-11-10 11:21:16 +0100) Fix getdents alignment issues (#704) Richard Henderson (4): linux-user: Split out do_getdents, do_getdents64 linux-user: Always use flexible arrays for dirent d_name linux-user: Fix member types of target_dirent64 linux-user: Rewrite do_getdents, do_getdents64 linux-user/syscall.c | 290 ++ linux-user/syscall_defs.h | 12 +- 2 files changed, 141 insertions(+), 161 deletions(-) -- 2.31.1
[PULL 4/4] linux-user: Rewrite do_getdents, do_getdents64
From: Richard Henderson
Always allocate host storage; this ensures that the struct
is sufficiently aligned for the host. Merge the three host
implementations of getdents via a few ifdefs. Utilize the
same method for do_getdents64.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/704
Signed-off-by: Richard Henderson
Message-Id: <20211107124845.1174791-5-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
linux-user/syscall.c | 243 ++-
1 file changed, 101 insertions(+), 142 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 499415ad81b8..01efd5773b9a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8140,172 +8140,131 @@ static int host_to_target_cpu_mask(const unsigned
long *host_mask,
}
#ifdef TARGET_NR_getdents
-static int do_getdents(abi_long arg1, abi_long arg2, abi_long arg3)
+static int do_getdents(abi_long arg1, abi_long arg2, abi_long count)
{
-int ret;
+g_autofree void *hdirp = NULL;
+void *tdirp;
+int hlen, hoff, toff;
+int hreclen, treclen;
+
+hdirp = g_try_malloc(count);
+if (!hdirp) {
+return -TARGET_ENOMEM;
+}
#ifdef EMULATE_GETDENTS_WITH_GETDENTS
-# if TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64
-struct target_dirent *target_dirp;
-struct linux_dirent *dirp;
-abi_long count = arg3;
+hlen = sys_getdents(arg1, hdirp, count);
+#else
+hlen = sys_getdents64(arg1, hdirp, count);
+#endif
-dirp = g_try_malloc(count);
-if (!dirp) {
-return -TARGET_ENOMEM;
+hlen = get_errno(hlen);
+if (is_error(hlen)) {
+return hlen;
}
-ret = get_errno(sys_getdents(arg1, dirp, count));
-if (!is_error(ret)) {
-struct linux_dirent *de;
-struct target_dirent *tde;
-int len = ret;
-int reclen, treclen;
-int count1, tnamelen;
-
-count1 = 0;
-de = dirp;
-target_dirp = lock_user(VERIFY_WRITE, arg2, count, 0);
-if (!target_dirp) {
-return -TARGET_EFAULT;
-}
-tde = target_dirp;
-while (len > 0) {
-reclen = de->d_reclen;
-tnamelen = reclen - offsetof(struct linux_dirent, d_name);
-assert(tnamelen >= 0);
-treclen = tnamelen + offsetof(struct target_dirent, d_name);
-assert(count1 + treclen <= count);
-tde->d_reclen = tswap16(treclen);
-tde->d_ino = tswapal(de->d_ino);
-tde->d_off = tswapal(de->d_off);
-memcpy(tde->d_name, de->d_name, tnamelen);
-de = (struct linux_dirent *)((char *)de + reclen);
-len -= reclen;
-tde = (struct target_dirent *)((char *)tde + treclen);
-count1 += treclen;
-}
-ret = count1;
-unlock_user(target_dirp, arg2, ret);
-}
-g_free(dirp);
-# else
-struct linux_dirent *dirp;
-abi_long count = arg3;
-
-dirp = lock_user(VERIFY_WRITE, arg2, count, 0);
-if (!dirp) {
+tdirp = lock_user(VERIFY_WRITE, arg2, count, 0);
+if (!tdirp) {
return -TARGET_EFAULT;
}
-ret = get_errno(sys_getdents(arg1, dirp, count));
-if (!is_error(ret)) {
-struct linux_dirent *de;
-int len = ret;
-int reclen;
-de = dirp;
-while (len > 0) {
-reclen = de->d_reclen;
-if (reclen > len) {
-break;
-}
-de->d_reclen = tswap16(reclen);
-tswapls(&de->d_ino);
-tswapls(&de->d_off);
-de = (struct linux_dirent *)((char *)de + reclen);
-len -= reclen;
-}
-}
-unlock_user(dirp, arg2, ret);
-# endif
+
+for (hoff = toff = 0; hoff < hlen; hoff += hreclen, toff += treclen) {
+#ifdef EMULATE_GETDENTS_WITH_GETDENTS
+struct linux_dirent *hde = hdirp + hoff;
#else
-/* Implement getdents in terms of getdents64 */
-struct linux_dirent64 *dirp;
-abi_long count = arg3;
+struct linux_dirent64 *hde = hdirp + hoff;
+#endif
+struct target_dirent *tde = tdirp + toff;
+int namelen;
+uint8_t type;
-dirp = lock_user(VERIFY_WRITE, arg2, count, 0);
-if (!dirp) {
-return -TARGET_EFAULT;
-}
-ret = get_errno(sys_getdents64(arg1, dirp, count));
-if (!is_error(ret)) {
/*
- * Convert the dirent64 structs to target dirent. We do this
- * in-place, since we can guarantee that a target_dirent is no
- * larger than a dirent64; however this means we have to be
- * careful to read everything before writing in the new format.
+ * If somehow the host dirent is smaller than the target dirent,
+ * then the host could return more dirent in the buffer than we
+ * can pass on to the host. This could be fixed by returning
+ * fewer dirent to the guest and lseek on the dirfd to reset the
+ * file pointer to the
[PATCH 2/5] linux-headers: update to 5.16-rc1
Signed-off-by: Paolo Bonzini
---
include/standard-headers/drm/drm_fourcc.h | 121 +-
include/standard-headers/linux/ethtool.h | 31 +
include/standard-headers/linux/fuse.h | 10 +-
include/standard-headers/linux/pci_regs.h | 6 +
include/standard-headers/linux/virtio_gpu.h | 18 ++-
include/standard-headers/linux/virtio_ids.h | 24
include/standard-headers/linux/virtio_vsock.h | 3 +-
linux-headers/asm-arm64/unistd.h | 1 +
linux-headers/asm-generic/unistd.h| 22 +++-
linux-headers/asm-mips/unistd_n32.h | 1 +
linux-headers/asm-mips/unistd_n64.h | 1 +
linux-headers/asm-mips/unistd_o32.h | 1 +
linux-headers/asm-powerpc/unistd_32.h | 1 +
linux-headers/asm-powerpc/unistd_64.h | 1 +
linux-headers/asm-s390/unistd_32.h| 1 +
linux-headers/asm-s390/unistd_64.h| 1 +
linux-headers/asm-x86/kvm.h | 5 +
linux-headers/asm-x86/unistd_32.h | 3 +
linux-headers/asm-x86/unistd_64.h | 3 +
linux-headers/asm-x86/unistd_x32.h| 3 +
linux-headers/linux/kvm.h | 40 +-
21 files changed, 276 insertions(+), 21 deletions(-)
diff --git a/include/standard-headers/drm/drm_fourcc.h
b/include/standard-headers/drm/drm_fourcc.h
index 352b51fd0a..2c025cb4fe 100644
--- a/include/standard-headers/drm/drm_fourcc.h
+++ b/include/standard-headers/drm/drm_fourcc.h
@@ -103,6 +103,12 @@ extern "C" {
/* 8 bpp Red */
#define DRM_FORMAT_R8 fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
+/* 10 bpp Red */
+#define DRM_FORMAT_R10 fourcc_code('R', '1', '0', ' ') /* [15:0] x:R
6:10 little endian */
+
+/* 12 bpp Red */
+#define DRM_FORMAT_R12 fourcc_code('R', '1', '2', ' ') /* [15:0] x:R
4:12 little endian */
+
/* 16 bpp Red */
#define DRM_FORMAT_R16 fourcc_code('R', '1', '6', ' ') /* [15:0] R
little endian */
@@ -372,6 +378,12 @@ extern "C" {
#define DRM_FORMAT_RESERVED ((1ULL << 56) - 1)
+#define fourcc_mod_get_vendor(modifier) \
+ (((modifier) >> 56) & 0xff)
+
+#define fourcc_mod_is_vendor(modifier, vendor) \
+ (fourcc_mod_get_vendor(modifier) == DRM_FORMAT_MOD_VENDOR_## vendor)
+
#define fourcc_mod_code(vendor, val) \
uint64_t)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | ((val) &
0x00ffULL))
@@ -899,9 +911,9 @@ drm_fourcc_canonicalize_nvidia_format_mod(uint64_t modifier)
/*
* The top 4 bits (out of the 56 bits alloted for specifying vendor specific
- * modifiers) denote the category for modifiers. Currently we have only two
- * categories of modifiers ie AFBC and MISC. We can have a maximum of sixteen
- * different categories.
+ * modifiers) denote the category for modifiers. Currently we have three
+ * categories of modifiers ie AFBC, MISC and AFRC. We can have a maximum of
+ * sixteen different categories.
*/
#define DRM_FORMAT_MOD_ARM_CODE(__type, __val) \
fourcc_mod_code(ARM, ((uint64_t)(__type) << 52) | ((__val) &
0x000fULL))
@@ -1016,6 +1028,109 @@ drm_fourcc_canonicalize_nvidia_format_mod(uint64_t
modifier)
*/
#define AFBC_FORMAT_MOD_USM(1ULL << 12)
+/*
+ * Arm Fixed-Rate Compression (AFRC) modifiers
+ *
+ * AFRC is a proprietary fixed rate image compression protocol and format,
+ * designed to provide guaranteed bandwidth and memory footprint
+ * reductions in graphics and media use-cases.
+ *
+ * AFRC buffers consist of one or more planes, with the same components
+ * and meaning as an uncompressed buffer using the same pixel format.
+ *
+ * Within each plane, the pixel/luma/chroma values are grouped into
+ * "coding unit" blocks which are individually compressed to a
+ * fixed size (in bytes). All coding units within a given plane of a buffer
+ * store the same number of values, and have the same compressed size.
+ *
+ * The coding unit size is configurable, allowing different rates of
compression.
+ *
+ * The start of each AFRC buffer plane must be aligned to an alignment granule
which
+ * depends on the coding unit size.
+ *
+ * Coding Unit Size Plane Alignment
+ * ---
+ * 16 bytes 1024 bytes
+ * 24 bytes 512 bytes
+ * 32 bytes 2048 bytes
+ *
+ * Coding units are grouped into paging tiles. AFRC buffer dimensions must be
aligned
+ * to a multiple of the paging tile dimensions.
+ * The dimensions of each paging tile depend on whether the buffer is
optimised for
+ * scanline (SCAN layout) or rotated (ROT layout) access.
+ *
+ * Layout Paging Tile Width Paging Tile Height
+ * -- - --
+ * SCAN 16 coding units 4 coding units
+ * ROT 8 coding units 8 coding units
+ *
+ * The dimensions of each coding unit depend on the number of components
+ * in the compressed plane and whether the buffer is optimised for
+ * scanline (SCAN la
[PULL 0/4] tcg patch queue
The following changes since commit 1b9fc6d8ba6667ceb56a3392e84656dcaed0d676: Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging (2021-11-11 09:56:22 +0100) are available in the Git repository at: https://gitlab.com/rth7680/qemu.git tags/pull-tcg-2021 for you to fetch changes up to d58f01733b94845b0c2232018a2bedb6a2347ec5: tcg/s390x: Fix tcg_out_vec_op argument type (2021-11-11 11:47:58 +0100) appease coverity vs extract2 update docs for ctpop opcodes tcg/s390x build fix for gcc11 Miroslav Rezanina (1): tcg/s390x: Fix tcg_out_vec_op argument type Philippe Mathieu-Daudé (1): tcg: Remove TCI experimental status Richard Henderson (2): tcg/optimize: Add an extra cast to fold_extract2 tcg: Document ctpop opcodes docs/about/build-platforms.rst | 10 ++ meson.build| 4 ++-- tcg/optimize.c | 2 +- tcg/s390x/tcg-target.c.inc | 3 ++- meson_options.txt | 2 +- scripts/meson-buildoptions.sh | 3 +-- tcg/README | 6 ++ 7 files changed, 19 insertions(+), 11 deletions(-)
[PULL 1/4] linux-user: Split out do_getdents, do_getdents64
From: Richard Henderson
Retain all 3 implementations of getdents for now.
Signed-off-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Reviewed by: Warner Losh
Message-Id: <20211107124845.1174791-2-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
linux-user/syscall.c | 325 +++
1 file changed, 172 insertions(+), 153 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 544f5b662ffe..a2f605dec4ca 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8137,6 +8137,176 @@ static int host_to_target_cpu_mask(const unsigned long
*host_mask,
return 0;
}
+#ifdef TARGET_NR_getdents
+static int do_getdents(abi_long arg1, abi_long arg2, abi_long arg3)
+{
+int ret;
+
+#ifdef EMULATE_GETDENTS_WITH_GETDENTS
+# if TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64
+struct target_dirent *target_dirp;
+struct linux_dirent *dirp;
+abi_long count = arg3;
+
+dirp = g_try_malloc(count);
+if (!dirp) {
+return -TARGET_ENOMEM;
+}
+
+ret = get_errno(sys_getdents(arg1, dirp, count));
+if (!is_error(ret)) {
+struct linux_dirent *de;
+struct target_dirent *tde;
+int len = ret;
+int reclen, treclen;
+int count1, tnamelen;
+
+count1 = 0;
+de = dirp;
+target_dirp = lock_user(VERIFY_WRITE, arg2, count, 0);
+if (!target_dirp) {
+return -TARGET_EFAULT;
+}
+tde = target_dirp;
+while (len > 0) {
+reclen = de->d_reclen;
+tnamelen = reclen - offsetof(struct linux_dirent, d_name);
+assert(tnamelen >= 0);
+treclen = tnamelen + offsetof(struct target_dirent, d_name);
+assert(count1 + treclen <= count);
+tde->d_reclen = tswap16(treclen);
+tde->d_ino = tswapal(de->d_ino);
+tde->d_off = tswapal(de->d_off);
+memcpy(tde->d_name, de->d_name, tnamelen);
+de = (struct linux_dirent *)((char *)de + reclen);
+len -= reclen;
+tde = (struct target_dirent *)((char *)tde + treclen);
+count1 += treclen;
+}
+ret = count1;
+unlock_user(target_dirp, arg2, ret);
+}
+g_free(dirp);
+# else
+struct linux_dirent *dirp;
+abi_long count = arg3;
+
+dirp = lock_user(VERIFY_WRITE, arg2, count, 0);
+if (!dirp) {
+return -TARGET_EFAULT;
+}
+ret = get_errno(sys_getdents(arg1, dirp, count));
+if (!is_error(ret)) {
+struct linux_dirent *de;
+int len = ret;
+int reclen;
+de = dirp;
+while (len > 0) {
+reclen = de->d_reclen;
+if (reclen > len) {
+break;
+}
+de->d_reclen = tswap16(reclen);
+tswapls(&de->d_ino);
+tswapls(&de->d_off);
+de = (struct linux_dirent *)((char *)de + reclen);
+len -= reclen;
+}
+}
+unlock_user(dirp, arg2, ret);
+# endif
+#else
+/* Implement getdents in terms of getdents64 */
+struct linux_dirent64 *dirp;
+abi_long count = arg3;
+
+dirp = lock_user(VERIFY_WRITE, arg2, count, 0);
+if (!dirp) {
+return -TARGET_EFAULT;
+}
+ret = get_errno(sys_getdents64(arg1, dirp, count));
+if (!is_error(ret)) {
+/*
+ * Convert the dirent64 structs to target dirent. We do this
+ * in-place, since we can guarantee that a target_dirent is no
+ * larger than a dirent64; however this means we have to be
+ * careful to read everything before writing in the new format.
+ */
+struct linux_dirent64 *de;
+struct target_dirent *tde;
+int len = ret;
+int tlen = 0;
+
+de = dirp;
+tde = (struct target_dirent *)dirp;
+while (len > 0) {
+int namelen, treclen;
+int reclen = de->d_reclen;
+uint64_t ino = de->d_ino;
+int64_t off = de->d_off;
+uint8_t type = de->d_type;
+
+namelen = strlen(de->d_name);
+treclen = offsetof(struct target_dirent, d_name) + namelen + 2;
+treclen = QEMU_ALIGN_UP(treclen, sizeof(abi_long));
+
+memmove(tde->d_name, de->d_name, namelen + 1);
+tde->d_ino = tswapal(ino);
+tde->d_off = tswapal(off);
+tde->d_reclen = tswap16(treclen);
+/*
+ * The target_dirent type is in what was formerly a padding
+ * byte at the end of the structure:
+ */
+*(((char *)tde) + treclen - 1) = type;
+
+de = (struct linux_dirent64 *)((char *)de + reclen);
+tde = (struct target_dirent *)((char *)tde + treclen);
+len -= reclen;
+tlen += treclen;
+}
+ret = tlen;
+}
+unlock_user(dirp, arg2, ret);
+#endif
+return ret;
+}
+#endif /* TARGET_NR_ge
[PULL 1/4] tcg/optimize: Add an extra cast to fold_extract2
There is no bug, but silence a warning about computation
in int32_t being assigned to a uint64_t.
Reported-by: Coverity CID 1465220
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Richard Henderson
---
tcg/optimize.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tcg/optimize.c b/tcg/optimize.c
index dbb2d46e88..2397f2cf93 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -1365,7 +1365,7 @@ static bool fold_extract2(OptContext *ctx, TCGOp *op)
v2 <<= 64 - shr;
} else {
v1 = (uint32_t)v1 >> shr;
-v2 = (int32_t)v2 << (32 - shr);
+v2 = (uint64_t)((int32_t)v2 << (32 - shr));
}
return tcg_opt_gen_movi(ctx, op, op->args[0], v1 | v2);
}
--
2.25.1
[PULL 2/4] linux-user: Always use flexible arrays for dirent d_name
From: Richard Henderson
We currently use a flexible array member for target_dirent,
but use incorrectly fixed length arrays for target_dirent64,
linux_dirent and linux_dirent64.
This requires that we adjust the definition of the VFAT READDIR
ioctls which hard-code the 256 namelen size into the ioctl constant.
Signed-off-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Reviewed by: Warner Losh
Message-Id: <20211107124845.1174791-3-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
linux-user/syscall.c | 6 --
linux-user/syscall_defs.h | 6 +++---
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a2f605dec4ca..499415ad81b8 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -197,8 +197,10 @@
//#define DEBUG_ERESTARTSYS
//#include
-#defineVFAT_IOCTL_READDIR_BOTH _IOR('r', 1, struct
linux_dirent [2])
-#defineVFAT_IOCTL_READDIR_SHORT_IOR('r', 2, struct
linux_dirent [2])
+#define VFAT_IOCTL_READDIR_BOTH \
+_IOC(_IOC_READ, 'r', 1, (sizeof(struct linux_dirent) + 256) * 2)
+#define VFAT_IOCTL_READDIR_SHORT \
+_IOC(_IOC_READ, 'r', 2, (sizeof(struct linux_dirent) + 256) * 2)
#undef _syscall0
#undef _syscall1
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index a5ce487dcc38..98b09ee6d656 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -441,7 +441,7 @@ struct target_dirent64 {
int64_t d_off;
unsigned short d_reclen;
unsigned char d_type;
- chard_name[256];
+ chard_name[];
};
@@ -2714,7 +2714,7 @@ struct linux_dirent {
longd_ino;
unsigned long d_off;
unsigned short d_reclen;
-chard_name[256]; /* We must not include limits.h! */
+chard_name[];
};
struct linux_dirent64 {
@@ -2722,7 +2722,7 @@ struct linux_dirent64 {
int64_t d_off;
unsigned short d_reclen;
unsigned char d_type;
-chard_name[256];
+chard_name[];
};
struct target_mq_attr {
--
2.31.1
[PULL 3/4] linux-user: Fix member types of target_dirent64
From: Richard Henderson
The host uint64_t (etc) does not have the correct
alignment constraint as the guest: use abi_* types.
Signed-off-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Reviewed by: Warner Losh
Message-Id: <20211107124845.1174791-4-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
linux-user/syscall_defs.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 98b09ee6d656..41aaafbac12c 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -437,9 +437,9 @@ struct target_dirent {
};
struct target_dirent64 {
- uint64_td_ino;
- int64_t d_off;
- unsigned short d_reclen;
+ abi_ullong d_ino;
+ abi_llong d_off;
+ abi_ushort d_reclen;
unsigned char d_type;
chard_name[];
};
--
2.31.1
[PATCH 0/5] Update linux-headers + NOIRQ support for KVM gdbstub
The first two patches update linux-headers to 5.16-rc1. This require a small change to virtio-gpu as well. The remaining three patches are a rework of Maxim's patch to support KVM_GUESTDBG_BLOCKIRQ, adjusted to not break compilation on non-x86 architectures. Maxim Levitsky (3): gdbstub: reject unsupported flags in handle_set_qemu_sstep gdbstub, kvm: let KVM report supported singlestep flags kvm: add support for KVM_GUESTDBG_BLOCKIRQ Paolo Bonzini (2): virtio-gpu: do not byteswap padding linux-headers: update to 5.16-rc1 accel/kvm/kvm-all.c | 29 + gdbstub.c | 81 include/hw/virtio/virtio-gpu-bswap.h | 1 - include/standard-headers/drm/drm_fourcc.h | 121 +- include/standard-headers/linux/ethtool.h | 31 + include/standard-headers/linux/fuse.h | 10 +- include/standard-headers/linux/pci_regs.h | 6 + include/standard-headers/linux/virtio_gpu.h | 18 ++- include/standard-headers/linux/virtio_ids.h | 24 include/standard-headers/linux/virtio_vsock.h | 3 +- include/sysemu/kvm.h | 15 +++ linux-headers/asm-arm64/unistd.h | 1 + linux-headers/asm-generic/unistd.h| 22 +++- linux-headers/asm-mips/unistd_n32.h | 1 + linux-headers/asm-mips/unistd_n64.h | 1 + linux-headers/asm-mips/unistd_o32.h | 1 + linux-headers/asm-powerpc/unistd_32.h | 1 + linux-headers/asm-powerpc/unistd_64.h | 1 + linux-headers/asm-s390/unistd_32.h| 1 + linux-headers/asm-s390/unistd_64.h| 1 + linux-headers/asm-x86/kvm.h | 5 + linux-headers/asm-x86/unistd_32.h | 3 + linux-headers/asm-x86/unistd_64.h | 3 + linux-headers/asm-x86/unistd_x32.h| 3 + linux-headers/linux/kvm.h | 40 +- 25 files changed, 375 insertions(+), 48 deletions(-) -- 2.33.1
[PATCH 1/5] virtio-gpu: do not byteswap padding
In Linux 5.16, the padding of struct virtio_gpu_ctrl_hdr has become a single-byte field followed by a uint8_t[3] array of padding bytes, and virtio_gpu_ctrl_hdr_bswap does not compile anymore. Signed-off-by: Paolo Bonzini --- include/hw/virtio/virtio-gpu-bswap.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/hw/virtio/virtio-gpu-bswap.h b/include/hw/virtio/virtio-gpu-bswap.h index e2bee8f595..5faac0d8d5 100644 --- a/include/hw/virtio/virtio-gpu-bswap.h +++ b/include/hw/virtio/virtio-gpu-bswap.h @@ -24,7 +24,6 @@ virtio_gpu_ctrl_hdr_bswap(struct virtio_gpu_ctrl_hdr *hdr) le32_to_cpus(&hdr->flags); le64_to_cpus(&hdr->fence_id); le32_to_cpus(&hdr->ctx_id); -le32_to_cpus(&hdr->padding); } static inline void -- 2.33.1
[PULL 2/4] tcg: Remove TCI experimental status
From: Philippe Mathieu-Daudé
The following commits (released in v6.0.0) made raised the
quality of the TCI backend to the other TCG architectures,
thus is is not considerated experimental anymore:
- c6fbea47664..2f74f45e32b
- dc09f047edd..9e9acb7b348
- b6139eb0578..2fc6f16ca5e
- dbcbda2cd84..5e8892db93f
Signed-off-by: Philippe Mathieu-Daudé
Message-Id: <20211106111457.517546-1-f4...@amsat.org>
Signed-off-by: Richard Henderson
---
docs/about/build-platforms.rst | 10 ++
meson.build| 4 ++--
meson_options.txt | 2 +-
scripts/meson-buildoptions.sh | 3 +--
4 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/docs/about/build-platforms.rst b/docs/about/build-platforms.rst
index bcb1549721..c29a4b8fe6 100644
--- a/docs/about/build-platforms.rst
+++ b/docs/about/build-platforms.rst
@@ -54,10 +54,12 @@ Those hosts are officially supported, with various
accelerators:
* - x86
- hax, hvf (64 bit only), kvm, nvmm, tcg, whpx (64 bit only), xen
-Other host architectures are not supported. It is possible to build QEMU on an
-unsupported host architecture using the configure ``--enable-tcg-interpreter``
-option to enable the experimental TCI support, but note that this is very slow
-and is not recommended.
+Other host architectures are not supported. It is possible to build QEMU system
+emulation on an unsupported host architecture using the configure
+``--enable-tcg-interpreter`` option to enable the TCI support, but note that
+this is very slow and is not recommended for normal use. QEMU user emulation
+requires host-specific support for signal handling, therefore TCI won't help
+on unsupported host architectures.
Non-supported architectures may be removed in the future following the
:ref:`deprecation process`.
diff --git a/meson.build b/meson.build
index 9702fdce6d..2ece4fe088 100644
--- a/meson.build
+++ b/meson.build
@@ -335,7 +335,7 @@ tcg_arch = config_host['ARCH']
if not get_option('tcg').disabled()
if cpu not in supported_cpus
if get_option('tcg_interpreter')
- warning('Unsupported CPU @0@, will use TCG with TCI (experimental and
slow)'.format(cpu))
+ warning('Unsupported CPU @0@, will use TCG with TCI (slow)'.format(cpu))
else
error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
endif
@@ -3290,7 +3290,7 @@ endif
summary_info += {'TCG support': config_all.has_key('CONFIG_TCG')}
if config_all.has_key('CONFIG_TCG')
if get_option('tcg_interpreter')
-summary_info += {'TCG backend': 'TCI (TCG with bytecode interpreter,
experimental and slow)'}
+summary_info += {'TCG backend': 'TCI (TCG with bytecode interpreter,
slow)'}
else
summary_info += {'TCG backend': 'native (@0@)'.format(cpu)}
endif
diff --git a/meson_options.txt b/meson_options.txt
index e740dce2a5..411952bc91 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -59,7 +59,7 @@ option('xen_pci_passthrough', type: 'feature', value: 'auto',
option('tcg', type: 'feature', value: 'auto',
description: 'TCG support')
option('tcg_interpreter', type: 'boolean', value: false,
- description: 'TCG with bytecode interpreter (experimental and slow)')
+ description: 'TCG with bytecode interpreter (slow)')
option('cfi', type: 'boolean', value: 'false',
description: 'Control-Flow Integrity (CFI)')
option('cfi_debug', type: 'boolean', value: 'false',
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 55b8a78560..45e1f2e20d 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -13,8 +13,7 @@ meson_options_help() {
printf "%s\n" ' jemalloc/system/tcmalloc)'
printf "%s\n" ' --enable-slirp[=CHOICE] Whether and how to find the slirp
library'
printf "%s\n" ' (choices:
auto/disabled/enabled/internal/system)'
- printf "%s\n" ' --enable-tcg-interpreter TCG with bytecode interpreter
(experimental and'
- printf "%s\n" ' slow)'
+ printf "%s\n" ' --enable-tcg-interpreter TCG with bytecode interpreter
(slow)'
printf "%s\n" ' --enable-trace-backends=CHOICE'
printf "%s\n" ' Set available tracing backends
[log] (choices:'
printf "%s\n" '
dtrace/ftrace/log/nop/simple/syslog/ust)'
--
2.25.1
[PATCH 3/5] gdbstub: reject unsupported flags in handle_set_qemu_sstep
From: Maxim Levitsky
handle_query_qemu_sstepbits is reporting NOIRQ and NOTIMER bits
even if they are not supported (as is the case with record/replay).
Instead, store the supported singlestep flags and reject
any unsupported bits in handle_set_qemu_sstep. This removes
the need for the get_sstep_flags() wrapper.
While at it, move the variables in GDBState, instead of using
global variables.
Signed-off-by: Maxim Levitsky
[Extracted from Maxim's patch into a separate commit. - Paolo]
Signed-off-by: Paolo Bonzini
---
gdbstub.c | 73 +++
1 file changed, 47 insertions(+), 26 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 23baaef40e..960b9fbcd0 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -368,27 +368,10 @@ typedef struct GDBState {
gdb_syscall_complete_cb current_syscall_cb;
GString *str_buf;
GByteArray *mem_buf;
+int sstep_flags;
+int supported_sstep_flags;
} GDBState;
-/* By default use no IRQs and no timers while single stepping so as to
- * make single stepping like an ICE HW step.
- */
-static int sstep_flags = SSTEP_ENABLE|SSTEP_NOIRQ|SSTEP_NOTIMER;
-
-/* Retrieves flags for single step mode. */
-static int get_sstep_flags(void)
-{
-/*
- * In replay mode all events written into the log should be replayed.
- * That is why NOIRQ flag is removed in this mode.
- */
-if (replay_mode != REPLAY_MODE_NONE) {
-return SSTEP_ENABLE;
-} else {
-return sstep_flags;
-}
-}
-
static GDBState gdbserver_state;
static void init_gdbserver_state(void)
@@ -399,6 +382,24 @@ static void init_gdbserver_state(void)
gdbserver_state.str_buf = g_string_new(NULL);
gdbserver_state.mem_buf = g_byte_array_sized_new(MAX_PACKET_LENGTH);
gdbserver_state.last_packet = g_byte_array_sized_new(MAX_PACKET_LENGTH +
4);
+
+/*
+ * In replay mode all events written into the log should be replayed.
+ * That is why NOIRQ flag is removed in this mode.
+ */
+if (replay_mode != REPLAY_MODE_NONE) {
+gdbserver_state.supported_sstep_flags = SSTEP_ENABLE;
+} else {
+gdbserver_state.supported_sstep_flags =
+SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
+}
+
+/*
+ * By default use no IRQs and no timers while single stepping so as to
+ * make single stepping like an ICE HW step.
+ */
+gdbserver_state.sstep_flags = gdbserver_state.supported_sstep_flags;
+
}
#ifndef CONFIG_USER_ONLY
@@ -505,7 +506,7 @@ static int gdb_continue_partial(char *newstates)
CPU_FOREACH(cpu) {
if (newstates[cpu->cpu_index] == 's') {
trace_gdbstub_op_stepping(cpu->cpu_index);
-cpu_single_step(cpu, sstep_flags);
+cpu_single_step(cpu, gdbserver_state.sstep_flags);
}
}
gdbserver_state.running_state = 1;
@@ -524,7 +525,7 @@ static int gdb_continue_partial(char *newstates)
break; /* nothing to do here */
case 's':
trace_gdbstub_op_stepping(cpu->cpu_index);
-cpu_single_step(cpu, get_sstep_flags());
+cpu_single_step(cpu, gdbserver_state.sstep_flags);
cpu_resume(cpu);
flag = 1;
break;
@@ -1883,7 +1884,7 @@ static void handle_step(GArray *params, void *user_ctx)
gdb_set_cpu_pc((target_ulong)get_param(params, 0)->val_ull);
}
-cpu_single_step(gdbserver_state.c_cpu, get_sstep_flags());
+cpu_single_step(gdbserver_state.c_cpu, gdbserver_state.sstep_flags);
gdb_continue();
}
@@ -2017,24 +2018,44 @@ static void handle_v_commands(GArray *params, void
*user_ctx)
static void handle_query_qemu_sstepbits(GArray *params, void *user_ctx)
{
-g_string_printf(gdbserver_state.str_buf, "ENABLE=%x,NOIRQ=%x,NOTIMER=%x",
-SSTEP_ENABLE, SSTEP_NOIRQ, SSTEP_NOTIMER);
+g_string_printf(gdbserver_state.str_buf, "ENABLE=%x", SSTEP_ENABLE);
+
+if (gdbserver_state.supported_sstep_flags & SSTEP_NOIRQ) {
+g_string_append_printf(gdbserver_state.str_buf, ",NOIRQ=%x",
+ SSTEP_NOIRQ);
+}
+
+if (gdbserver_state.supported_sstep_flags & SSTEP_NOTIMER) {
+g_string_append_printf(gdbserver_state.str_buf, ",NOTIMER=%x",
+ SSTEP_NOTIMER);
+}
+
put_strbuf();
}
static void handle_set_qemu_sstep(GArray *params, void *user_ctx)
{
+int new_sstep_flags;
+
if (!params->len) {
return;
}
-sstep_flags = get_param(params, 0)->val_ul;
+new_sstep_flags = get_param(params, 0)->val_ul;
+
+if (new_sstep_flags & ~gdbserver_state.supported_sstep_flags) {
+put_packet("E22");
+return;
+}
+
+gdbserver_state.sstep_flags = new_sstep_flags;
put_packet("OK");
}
static void handle_query_qemu_sstep(GArray *params, void *user_ctx)
{
-g_string_printf(gdbserver_state.str_buf, "0x%x", sstep_flags);
+g_str
[PATCH 4/5] gdbstub, kvm: let KVM report supported singlestep flags
From: Maxim Levitsky
Signed-off-by: Maxim Levitsky
[Extracted from Maxim's patch into a separate commit. - Paolo]
Signed-off-by: Paolo Bonzini
---
accel/kvm/kvm-all.c | 12
gdbstub.c| 10 +-
include/sysemu/kvm.h | 15 +++
3 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index eecd8031cf..2f5597572a 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -168,6 +168,8 @@ bool kvm_vm_attributes_allowed;
bool kvm_direct_msi_allowed;
bool kvm_ioeventfd_any_length_allowed;
bool kvm_msi_use_devid;
+bool kvm_has_guest_debug;
+int kvm_sstep_flags;
static bool kvm_immediate_exit;
static hwaddr kvm_max_slot_size = ~0;
@@ -2564,6 +2566,16 @@ static int kvm_init(MachineState *ms)
kvm_ioeventfd_any_length_allowed =
(kvm_check_extension(s, KVM_CAP_IOEVENTFD_ANY_LENGTH) > 0);
+#ifdef KVM_CAP_SET_GUEST_DEBUG
+kvm_has_guest_debug =
+(kvm_check_extension(s, KVM_CAP_SET_GUEST_DEBUG) > 0);
+#endif
+
+kvm_sstep_flags = 0;
+if (kvm_has_guest_debug) {
+kvm_sstep_flags = SSTEP_ENABLE;
+}
+
kvm_state = s;
ret = kvm_arch_init(ms, s);
diff --git a/gdbstub.c b/gdbstub.c
index 960b9fbcd0..f961d68e16 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -389,6 +389,8 @@ static void init_gdbserver_state(void)
*/
if (replay_mode != REPLAY_MODE_NONE) {
gdbserver_state.supported_sstep_flags = SSTEP_ENABLE;
+} else if (kvm_enabled()) {
+gdbserver_state.supported_sstep_flags =
kvm_get_supported_sstep_flags();
} else {
gdbserver_state.supported_sstep_flags =
SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
@@ -398,7 +400,8 @@ static void init_gdbserver_state(void)
* By default use no IRQs and no timers while single stepping so as to
* make single stepping like an ICE HW step.
*/
-gdbserver_state.sstep_flags = gdbserver_state.supported_sstep_flags;
+gdbserver_state.sstep_flags = SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
+gdbserver_state.sstep_flags &= gdbserver_state.supported_sstep_flags;
}
@@ -3518,6 +3521,11 @@ int gdbserver_start(const char *device)
return -1;
}
+if (kvm_enabled() && !kvm_supports_guest_debug()) {
+error_report("gdbstub: KVM doesn't support guest debugging");
+return -1;
+}
+
if (!device)
return -1;
if (strcmp(device, "none") != 0) {
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 7b22aeb6ae..6eb39a088b 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -46,6 +46,8 @@ extern bool kvm_readonly_mem_allowed;
extern bool kvm_direct_msi_allowed;
extern bool kvm_ioeventfd_any_length_allowed;
extern bool kvm_msi_use_devid;
+extern bool kvm_has_guest_debug;
+extern int kvm_sstep_flags;
#define kvm_enabled() (kvm_allowed)
/**
@@ -167,6 +169,17 @@ extern bool kvm_msi_use_devid;
*/
#define kvm_msi_devid_required() (kvm_msi_use_devid)
+/*
+ * Does KVM support guest debugging
+ */
+#define kvm_supports_guest_debug() (kvm_has_guest_debug)
+
+/*
+ * kvm_supported_sstep_flags
+ * Returns: SSTEP_* flags that KVM supports for guest debug
+ */
+#define kvm_get_supported_sstep_flags() (kvm_sstep_flags)
+
#else
#define kvm_enabled() (0)
@@ -184,6 +197,8 @@ extern bool kvm_msi_use_devid;
#define kvm_direct_msi_enabled() (false)
#define kvm_ioeventfd_any_length_enabled() (false)
#define kvm_msi_devid_required() (false)
+#define kvm_supports_guest_debug() (false)
+#define kvm_get_supported_sstep_flags() (0)
#endif /* CONFIG_KVM_IS_POSSIBLE */
--
2.33.1
[PATCH 5/5] kvm: add support for KVM_GUESTDBG_BLOCKIRQ
From: Maxim Levitsky
Use the KVM_GUESTDBG_BLOCKIRQ debug flag if supported.
Signed-off-by: Maxim Levitsky
[Extracted from Maxim's patch into a separate commit. - Paolo]
Signed-off-by: Paolo Bonzini
---
accel/kvm/kvm-all.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 2f5597572a..0e66ebb497 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -61,6 +61,10 @@
#endif
#define PAGE_SIZE qemu_real_host_page_size
+#ifndef KVM_GUESTDBG_BLOCKIRQ
+#define KVM_GUESTDBG_BLOCKIRQ 0
+#endif
+
//#define DEBUG_KVM
#ifdef DEBUG_KVM
@@ -2574,6 +2578,15 @@ static int kvm_init(MachineState *ms)
kvm_sstep_flags = 0;
if (kvm_has_guest_debug) {
kvm_sstep_flags = SSTEP_ENABLE;
+
+#if defined KVM_CAP_SET_GUEST_DEBUG2
+int guest_debug_flags =
+kvm_check_extension(s, KVM_CAP_SET_GUEST_DEBUG2);
+
+if (guest_debug_flags & KVM_GUESTDBG_BLOCKIRQ) {
+kvm_sstep_flags |= SSTEP_NOIRQ;
+}
+#endif
}
kvm_state = s;
@@ -3205,6 +3218,10 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long
reinject_trap)
if (cpu->singlestep_enabled) {
data.dbg.control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+
+if (cpu->singlestep_enabled & SSTEP_NOIRQ) {
+data.dbg.control |= KVM_GUESTDBG_BLOCKIRQ;
+}
}
kvm_arch_update_guest_debug(cpu, &data.dbg);
--
2.33.1
[PULL 3/4] tcg: Document ctpop opcodes
Fixes: a768e4e99247 Resolves: https://gitlab.com/qemu-project/qemu/-/issues/658 Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- tcg/README | 6 ++ 1 file changed, 6 insertions(+) diff --git a/tcg/README b/tcg/README index c2e7762a37..bc15cc3b32 100644 --- a/tcg/README +++ b/tcg/README @@ -254,6 +254,12 @@ t0 = t1 ? clz(t1) : t2 t0 = t1 ? ctz(t1) : t2 +* ctpop_i32/i64 t0, t1 + +t0 = number of bits set in t1 +With "ctpop" short for "count population", matching +the function name used in include/qemu/host-utils.h. + * Shifts/Rotates * shl_i32/i64 t0, t1, t2 -- 2.25.1
[PULL 4/4] tcg/s390x: Fix tcg_out_vec_op argument type
From: Miroslav Rezanina
Newly defined tcg_out_vec_op (34ef767609 tcg/s390x: Add host vector framework)
for s390x uses pointer argument definition.
This fails on gcc 11 as original declaration uses array argument:
In file included from ../tcg/tcg.c:430:
/builddir/build/BUILD/qemu-6.1.50/tcg/s390x/tcg-target.c.inc:2702:42: error:
argument 5 of type 'const TCGArg *' {aka 'const long unsigned int *'} declared
as a pointer [-Werror=array-parameter=]
2702 |const TCGArg *args, const int *const_args)
|~~^~~~
../tcg/tcg.c:121:41: note: previously declared as an array 'const TCGArg[16]'
{aka 'const long unsigned int[16]'}
121 |const TCGArg args[TCG_MAX_OP_ARGS],
|~^
In file included from ../tcg/tcg.c:430:
/builddir/build/BUILD/qemu-6.1.50/tcg/s390x/tcg-target.c.inc:2702:59: error:
argument 6 of type 'const int *' declared as a pointer
[-Werror=array-parameter=]
2702 |const TCGArg *args, const int *const_args)
|~~~^~
../tcg/tcg.c:122:38: note: previously declared as an array 'const int[16]'
122 |const int const_args[TCG_MAX_OP_ARGS]);
|~~^~~
Fixing argument type to pass build.
Signed-off-by: Miroslav Rezanina
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
Acked-by: David Hildenbrand
Message-Id: <20211027085629.240704-1-mreza...@redhat.com>
Signed-off-by: Richard Henderson
---
tcg/s390x/tcg-target.c.inc | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tcg/s390x/tcg-target.c.inc b/tcg/s390x/tcg-target.c.inc
index 8938c446c8..57e803e339 100644
--- a/tcg/s390x/tcg-target.c.inc
+++ b/tcg/s390x/tcg-target.c.inc
@@ -2699,7 +2699,8 @@ static void tcg_out_dupi_vec(TCGContext *s, TCGType type,
unsigned vece,
static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
unsigned vecl, unsigned vece,
- const TCGArg *args, const int *const_args)
+ const TCGArg args[TCG_MAX_OP_ARGS],
+ const int const_args[TCG_MAX_OP_ARGS])
{
TCGType type = vecl + TCG_TYPE_V64;
TCGArg a0 = args[0], a1 = args[1], a2 = args[2];
--
2.25.1
Re: [PATCH v3 07/20] target/riscv: Adjust csr write mask with XLEN
On 11/11/21 6:57 AM, LIU Zhiwei wrote: Write mask is representing the bits we care about. Signed-off-by: LIU Zhiwei --- target/riscv/insn_trans/trans_rvi.c.inc | 4 ++-- target/riscv/op_helper.c| 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v3 09/20] target/riscv: Alloc tcg global for cur_pm[mask|base]
On 11/11/21 6:57 AM, LIU Zhiwei wrote: Replace the array of pm_mask/pm_base with scalar variables. Remove the cached array value in DisasContext. Signed-off-by: LIU Zhiwei --- target/riscv/translate.c | 32 1 file changed, 8 insertions(+), 24 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v3 04/20] target/riscv: Extend pc for runtime pc write
On 11/11/21 6:57 AM, LIU Zhiwei wrote: In some cases, we must restore the guest PC to the address of the start of the TB, such as when the instruction counter hits zero. So extend pc register according to current xlen for these cases. Signed-off-by: LIU Zhiwei --- target/riscv/cpu.c| 22 +++--- target/riscv/cpu.h| 2 ++ target/riscv/cpu_helper.c | 2 +- 3 files changed, 22 insertions(+), 4 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH 1/5] virtio-gpu: do not byteswap padding
On Thu, Nov 11 2021, Paolo Bonzini wrote: > In Linux 5.16, the padding of struct virtio_gpu_ctrl_hdr has become a > single-byte field followed by a uint8_t[3] array of padding bytes, > and virtio_gpu_ctrl_hdr_bswap does not compile anymore. > > Signed-off-by: Paolo Bonzini > --- > include/hw/virtio/virtio-gpu-bswap.h | 1 - > 1 file changed, 1 deletion(-) Looks like the right way to adapt to the changes. Acked-by: Cornelia Huck
Re: [PATCH v3 10/20] target/riscv: Calculate address according to XLEN
On 11/11/21 6:57 AM, LIU Zhiwei wrote: Define one common function to compute a canonical address from a register plus offset. Merge gen_pm_adjust_address into this function. Signed-off-by: LIU Zhiwei --- target/riscv/insn_trans/trans_rva.c.inc | 9 +++-- target/riscv/insn_trans/trans_rvd.c.inc | 19 ++--- target/riscv/insn_trans/trans_rvf.c.inc | 19 ++--- target/riscv/insn_trans/trans_rvi.c.inc | 18 ++--- target/riscv/translate.c| 27 - 5 files changed, 22 insertions(+), 70 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v3 11/20] target/riscv: Split pm_enabled into mask and base
On 11/11/21 6:57 AM, LIU Zhiwei wrote: Use cached cur_pmmask and cur_pmbase to infer the current PM mode. This may decrease the TCG IR by one when pm_enabled is true and pm_base_enabled is false. Signed-off-by: LIU Zhiwei --- target/riscv/cpu.h| 3 ++- target/riscv/cpu_helper.c | 25 +++-- target/riscv/translate.c | 12 3 files changed, 17 insertions(+), 23 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH 2/5] linux-headers: update to 5.16-rc1
On Thu, Nov 11 2021, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini > --- > include/standard-headers/drm/drm_fourcc.h | 121 +- > include/standard-headers/linux/ethtool.h | 31 + > include/standard-headers/linux/fuse.h | 10 +- > include/standard-headers/linux/pci_regs.h | 6 + > include/standard-headers/linux/virtio_gpu.h | 18 ++- > include/standard-headers/linux/virtio_ids.h | 24 > include/standard-headers/linux/virtio_vsock.h | 3 +- > linux-headers/asm-arm64/unistd.h | 1 + > linux-headers/asm-generic/unistd.h| 22 +++- > linux-headers/asm-mips/unistd_n32.h | 1 + > linux-headers/asm-mips/unistd_n64.h | 1 + > linux-headers/asm-mips/unistd_o32.h | 1 + > linux-headers/asm-powerpc/unistd_32.h | 1 + > linux-headers/asm-powerpc/unistd_64.h | 1 + > linux-headers/asm-s390/unistd_32.h| 1 + > linux-headers/asm-s390/unistd_64.h| 1 + > linux-headers/asm-x86/kvm.h | 5 + > linux-headers/asm-x86/unistd_32.h | 3 + > linux-headers/asm-x86/unistd_64.h | 3 + > linux-headers/asm-x86/unistd_x32.h| 3 + > linux-headers/linux/kvm.h | 40 +- > 21 files changed, 276 insertions(+), 21 deletions(-) LGTM. Acked-by: Cornelia Huck
Re: [PATCH v3 12/20] target/riscv: Split out the vill from vtype
On 11/11/21 6:57 AM, LIU Zhiwei wrote:
We need not specially process vtype when XLEN changes.
Signed-off-by: LIU Zhiwei
---
target/riscv/cpu.h | 1 +
target/riscv/cpu_helper.c| 3 +--
target/riscv/csr.c | 13 -
target/riscv/machine.c | 1 +
target/riscv/vector_helper.c | 3 ++-
5 files changed, 17 insertions(+), 4 deletions(-)
Reviewed-by: Richard Henderson
@@ -104,6 +104,7 @@ static const VMStateDescription vmstate_vector = {
VMSTATE_UINTTL(env.vl, RISCVCPU),
VMSTATE_UINTTL(env.vstart, RISCVCPU),
VMSTATE_UINTTL(env.vtype, RISCVCPU),
+VMSTATE_BOOL(env.vill, RISCVCPU),
VMSTATE_END_OF_LIST()
Need to bump version.
r~
Re: [PATCH 1/5] virtio-gpu: do not byteswap padding
On 11/11/21 12:06, Paolo Bonzini wrote: > In Linux 5.16, the padding of struct virtio_gpu_ctrl_hdr has become a > single-byte field followed by a uint8_t[3] array of padding bytes, > and virtio_gpu_ctrl_hdr_bswap does not compile anymore. > > Signed-off-by: Paolo Bonzini > --- > include/hw/virtio/virtio-gpu-bswap.h | 1 - > 1 file changed, 1 deletion(-) :) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v3 13/20] target/riscv: Fix RESERVED field length in VTYPE
On 11/11/21 12:33 PM, Richard Henderson wrote: On 11/11/21 6:57 AM, LIU Zhiwei wrote: Signed-off-by: LIU Zhiwei --- target/riscv/cpu.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 52ce670cbe..b48c7c346c 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -105,7 +105,7 @@ typedef struct CPURISCVState CPURISCVState; FIELD(VTYPE, VLMUL, 0, 2) FIELD(VTYPE, VSEW, 2, 3) FIELD(VTYPE, VEDIV, 5, 2) -FIELD(VTYPE, RESERVED, 7, sizeof(target_ulong) * 8 - 9) +FIELD(VTYPE, RESERVED, 7, sizeof(target_ulong) * 8 - 8) Or better, remove it in the next patch. Bah, nevermind. Reviewed-by: Richard Henderson r~
Re: [PATCH for 6.2 v2 5/5] bios-tables-test: Update golden binaries
On Thu, 11 Nov 2021 03:34:37 -0500
"Michael S. Tsirkin" wrote:
> On Wed, Nov 10, 2021 at 04:11:40PM -0500, Igor Mammedov wrote:
> > From: Julia Suvorova
> >
> > The changes are the result of
> > 'hw/i386/acpi-build: Deny control on PCIe Native Hot-Plug in _OSC'
> > and listed here:
> >
> > Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities
> > {
> > CreateDWordField (Arg3, Zero, CDW1)
> > If ((Arg0 == ToUUID
> > ("33db4d5b-1ff7-401c-9657-7441c03dd766") /* PCI Host Bridge Device */))
> > {
> > CreateDWordField (Arg3, 0x04, CDW2)
> > CreateDWordField (Arg3, 0x08, CDW3)
> > Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> > -Local0 &= 0x1F
> > +Local0 &= 0x1E
> >
> > Signed-off-by: Julia Suvorova
> > Signed-off-by: Igor Mammedov
> > ---
> > tests/qtest/bios-tables-test-allowed-diff.h | 16
> > tests/data/acpi/q35/DSDT| Bin 8289 -> 8289 bytes
> > tests/data/acpi/q35/DSDT.acpihmat | Bin 9614 -> 9614 bytes
> > tests/data/acpi/q35/DSDT.bridge | Bin 11003 -> 11003 bytes
> > tests/data/acpi/q35/DSDT.cphp | Bin 8753 -> 8753 bytes
> > tests/data/acpi/q35/DSDT.dimmpxm| Bin 9943 -> 9943 bytes
> > tests/data/acpi/q35/DSDT.dmar | Bin 0 -> 8289 bytes
> > tests/data/acpi/q35/DSDT.ipmibt | Bin 8364 -> 8364 bytes
> > tests/data/acpi/q35/DSDT.ivrs | Bin 8306 -> 8306 bytes
> > tests/data/acpi/q35/DSDT.memhp | Bin 9648 -> 9648 bytes
> > tests/data/acpi/q35/DSDT.mmio64 | Bin 9419 -> 9419 bytes
> > tests/data/acpi/q35/DSDT.multi-bridge | Bin 8583 -> 8583 bytes
> > tests/data/acpi/q35/DSDT.nohpet | Bin 8147 -> 8147 bytes
> > tests/data/acpi/q35/DSDT.nosmm | Bin 0 -> 8289 bytes
> > tests/data/acpi/q35/DSDT.numamem| Bin 8295 -> 8295 bytes
> > tests/data/acpi/q35/DSDT.smm-compat | Bin 0 -> 8289 bytes
> > tests/data/acpi/q35/DSDT.smm-compat-nosmm | Bin 0 -> 8289 bytes
> > tests/data/acpi/q35/DSDT.tis.tpm12 | Bin 8894 -> 8894 bytes
> > tests/data/acpi/q35/DSDT.tis.tpm2 | Bin 8894 -> 8894 bytes
> > tests/data/acpi/q35/DSDT.xapic | Bin 35652 -> 35652 bytes
>
> Why do we have all the new files? What is going on here?
I think new files are not necessary.
I can update patch if we decide to keep ACPI enabled by default.
So question is:
do we revert to native pcie or stay with apci hootplug for 6.2?
>
> > 20 files changed, 16 deletions(-)
> > create mode 100644 tests/data/acpi/q35/DSDT.dmar
> > create mode 100644 tests/data/acpi/q35/DSDT.nosmm
> > create mode 100644 tests/data/acpi/q35/DSDT.smm-compat
> > create mode 100644 tests/data/acpi/q35/DSDT.smm-compat-nosmm
> >
> > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h
> > b/tests/qtest/bios-tables-test-allowed-diff.h
> > index 48e5634d4b..dfb8523c8b 100644
> > --- a/tests/qtest/bios-tables-test-allowed-diff.h
> > +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> > @@ -1,17 +1 @@
> > /* List of comma-separated changed AML files to ignore */
> > -"tests/data/acpi/q35/DSDT",
> > -"tests/data/acpi/q35/DSDT.tis",
> > -"tests/data/acpi/q35/DSDT.bridge",
> > -"tests/data/acpi/q35/DSDT.mmio64",
> > -"tests/data/acpi/q35/DSDT.ipmibt",
> > -"tests/data/acpi/q35/DSDT.cphp",
> > -"tests/data/acpi/q35/DSDT.memhp",
> > -"tests/data/acpi/q35/DSDT.acpihmat",
> > -"tests/data/acpi/q35/DSDT.numamem",
> > -"tests/data/acpi/q35/DSDT.dimmpxm",
> > -"tests/data/acpi/q35/DSDT.nohpet",
> > -"tests/data/acpi/q35/DSDT.tis.tpm2",
> > -"tests/data/acpi/q35/DSDT.tis.tpm12",
> > -"tests/data/acpi/q35/DSDT.multi-bridge",
> > -"tests/data/acpi/q35/DSDT.ivrs",
> > -"tests/data/acpi/q35/DSDT.xapic",
> > diff --git a/tests/data/acpi/q35/DSDT b/tests/data/acpi/q35/DSDT
> > index
> > 281fc82c03b2562d2e6b7caec0d817b034a47138..c1965f6051ef2af81dd8412abe169d87845bb033
> > 100644
> > GIT binary patch
> > delta 24
> > gcmaFp@X&$FCDET<0BnZ{w*UYD
> >
> > delta 24
> > gcmaFp@X&$FCDET<0BnK?w*UYD
> >
> > diff --git a/tests/data/acpi/q35/DSDT.acpihmat
> > b/tests/data/acpi/q35/DSDT.acpihmat
> > index
> > 8c1e05a11a328ec1cc6f86e36e52c28f41f9744e..f24d4874bff8d327a165ed7c36de507aea114edd
> > 100644
> > GIT binary patch
> > delta 24
> > fcmeD4?(^ny33dtTQ)OUa+&+=(3ZvY{`|DKzU@Hhn
> >
> > delta 24
> > fcmeD4?(^ny33dtTQ)OUa+%}Qx3ZwkS`|DKzU?vDi
> >
> > diff --git a/tests/data/acpi/q35/DSDT.bridge
> > b/tests/data/acpi/q35/DSDT.bridge
> > index
> > 6f1464b6c712d7f33cb4b891b7ce76fe228f44c9..424d51bd1cb39ea73501ef7d0044ee52cec5bdac
> > 100644
> > GIT binary patch
> > delta 24
> > gcmewz`a6`%CD >
> > delta 24
> > gcmewz`a6`%CD >
> > diff --git a/tests/data/acpi/q35/DSDT.cphp b/tests/data/acpi/q35/DSDT.cphp
> > index
> > f8337ff5191a37a47dcf7c09a6c39c4e704a15bf
Re: [PATCH v3 13/20] target/riscv: Fix RESERVED field length in VTYPE
On 11/11/21 6:57 AM, LIU Zhiwei wrote: Signed-off-by: LIU Zhiwei --- target/riscv/cpu.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 52ce670cbe..b48c7c346c 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -105,7 +105,7 @@ typedef struct CPURISCVState CPURISCVState; FIELD(VTYPE, VLMUL, 0, 2) FIELD(VTYPE, VSEW, 2, 3) FIELD(VTYPE, VEDIV, 5, 2) -FIELD(VTYPE, RESERVED, 7, sizeof(target_ulong) * 8 - 9) +FIELD(VTYPE, RESERVED, 7, sizeof(target_ulong) * 8 - 8) Or better, remove it in the next patch. r~
Re: [PATCH v3 15/20] target/riscv: Remove VILL field in VTYPE
On 11/11/21 6:57 AM, LIU Zhiwei wrote: Signed-off-by: LIU Zhiwei --- target/riscv/cpu.h | 1 - 1 file changed, 1 deletion(-) diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index b48c7c346c..5f35217f7d 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -106,7 +106,6 @@ FIELD(VTYPE, VLMUL, 0, 2) FIELD(VTYPE, VSEW, 2, 3) FIELD(VTYPE, VEDIV, 5, 2) FIELD(VTYPE, RESERVED, 7, sizeof(target_ulong) * 8 - 8) -FIELD(VTYPE, VILL, sizeof(target_ulong) * 8 - 1, 1) Hmm. It's sorta-kinda documentation. Otherwise we might as well remove RESERVED as well, and just move the 7 constant into vsetvl. Either way, Acked-by: Richard Henderson r~
Re: [PATCH v3 14/20] target/riscv: Adjust vsetvl according to XLEN
On 11/11/21 6:57 AM, LIU Zhiwei wrote: -DEF_HELPER_3(vsetvl, tl, env, tl, tl) +DEF_HELPER_4(vsetvl, tl, env, tl, tl, tl) Might as well make the argument i32, or don't pass it at all and use cpu_get_xl(). Otherwise, Reviewed-by: Richard Henderson r~
Re: [PATCH 3/5] gdbstub: reject unsupported flags in handle_set_qemu_sstep
On 11/11/21 12:06, Paolo Bonzini wrote:
> From: Maxim Levitsky
>
> handle_query_qemu_sstepbits is reporting NOIRQ and NOTIMER bits
> even if they are not supported (as is the case with record/replay).
> Instead, store the supported singlestep flags and reject
> any unsupported bits in handle_set_qemu_sstep. This removes
> the need for the get_sstep_flags() wrapper.
>
> While at it, move the variables in GDBState, instead of using
> global variables.
>
> Signed-off-by: Maxim Levitsky
> [Extracted from Maxim's patch into a separate commit. - Paolo]
> Signed-off-by: Paolo Bonzini
> ---
> gdbstub.c | 73 +++
> 1 file changed, 47 insertions(+), 26 deletions(-)
> static void init_gdbserver_state(void)
> @@ -399,6 +382,24 @@ static void init_gdbserver_state(void)
> gdbserver_state.str_buf = g_string_new(NULL);
> gdbserver_state.mem_buf = g_byte_array_sized_new(MAX_PACKET_LENGTH);
> gdbserver_state.last_packet = g_byte_array_sized_new(MAX_PACKET_LENGTH +
> 4);
Simpler:
gdbserver_state.supported_sstep_flags = SSTEP_ENABLE;
> +/*
> + * In replay mode all events written into the log should be replayed.
> + * That is why NOIRQ flag is removed in this mode.
> + */
if (replay_mode == REPLAY_MODE_NONE) {
gdbserver_state.supported_sstep_flags |= SSTEP_NOIRQ | SSTEP_NOTIMER;
}
> +if (replay_mode != REPLAY_MODE_NONE) {
> +gdbserver_state.supported_sstep_flags = SSTEP_ENABLE;
> +} else {
> +gdbserver_state.supported_sstep_flags =
> +SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
> +}
> +
> +/*
> + * By default use no IRQs and no timers while single stepping so as to
> + * make single stepping like an ICE HW step.
> + */
> +gdbserver_state.sstep_flags = gdbserver_state.supported_sstep_flags;
> +
> }
> static void handle_set_qemu_sstep(GArray *params, void *user_ctx)
> {
> +int new_sstep_flags;
> +
> if (!params->len) {
> return;
> }
>
> -sstep_flags = get_param(params, 0)->val_ul;
> +new_sstep_flags = get_param(params, 0)->val_ul;
> +
> +if (new_sstep_flags & ~gdbserver_state.supported_sstep_flags) {
> +put_packet("E22");
> +return;
> +}
Please does this change in a separate patch, after moving
to GDBState.
> +gdbserver_state.sstep_flags = new_sstep_flags;
> put_packet("OK");
> }
Re: [PATCH v3 19/20] target/riscv: Adjust scalar reg in vector with XLEN
On 11/11/21 6:57 AM, LIU Zhiwei wrote:
@@ -2670,6 +2672,7 @@ static bool trans_vmv_s_x(DisasContext *s, arg_vmv_s_x *a)
/* This instruction ignores LMUL and vector register groups */
int maxsz = s->vlen >> 3;
TCGv_i64 t1;
+TCGv src1 = get_gpr(s, a->rs1, EXT_ZERO);
A reminder that this is zero-extend for v0.7.1 and sign-extend for v1.0.0.
@@ -2679,7 +2682,7 @@ static bool trans_vmv_s_x(DisasContext *s, arg_vmv_s_x *a)
}
t1 = tcg_temp_new_i64();
-tcg_gen_extu_tl_i64(t1, cpu_gpr[a->rs1]);
+tcg_gen_extu_tl_i64(t1, src1);
Likewise.
vec_element_storei(s, a->rd, 0, t1);
tcg_temp_free_i64(t1);
done:
@@ -2748,12 +2751,28 @@ static bool slideup_check(DisasContext *s, arg_rmrr *a)
(a->rd != a->rs2));
}
+/* OPIVXU without GVEC IR */
+#define GEN_OPIVXU_TRANS(NAME, CHECK)\
+static bool trans_##NAME(DisasContext *s, arg_rmrr *a) \
+{\
+if (CHECK(s, a)) { \
+static gen_helper_opivx * const fns[4] = { \
+gen_helper_##NAME##_b, gen_helper_##NAME##_h,\
+gen_helper_##NAME##_w, gen_helper_##NAME##_d,\
+}; \
+ \
+return opivx_trans(a->rd, a->rs1, a->rs2, a->vm, \
+ fns[s->sew], s, EXT_ZERO);\
+}\
+return false;\
+}
+
GEN_OPIVX_TRANS(vslideup_vx, slideup_check)
-GEN_OPIVX_TRANS(vslide1up_vx, slideup_check)
+GEN_OPIVXU_TRANS(vslide1up_vx, slideup_check)
GEN_OPIVI_TRANS(vslideup_vi, 1, vslideup_vx, slideup_check)
GEN_OPIVX_TRANS(vslidedown_vx, opivx_check)
-GEN_OPIVX_TRANS(vslide1down_vx, opivx_check)
+GEN_OPIVXU_TRANS(vslide1down_vx, opivx_check)
GEN_OPIVI_TRANS(vslidedown_vi, 1, vslidedown_vx, opivx_check)
Likewise.
So if this patch set goes in after rvv 1.0, this whole patch may be dropped.
r~
Re: [PATCH v3 20/20] target/riscv: Enable uxl field write
On 11/11/21 6:58 AM, LIU Zhiwei wrote:
Signed-off-by: LIU Zhiwei
---
target/riscv/csr.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 8f8f170768..43eaa6c710 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -553,15 +553,14 @@ static RISCVException write_mstatus(CPURISCVState *env,
int csrno,
* RV32: MPV and GVA are not in mstatus. The current plan is to
* add them to mstatush. For now, we just don't support it.
*/
-mask |= MSTATUS_MPV | MSTATUS_GVA;
+mask |= MSTATUS_MPV | MSTATUS_GVA | MSTATUS64_UXL;
}
mstatus = (mstatus & ~mask) | (val & mask);
if (riscv_cpu_mxl(env) == MXL_RV64) {
-/* SXL and UXL fields are for now read only */
+/* SXL fields are for now read only */
mstatus = set_field(mstatus, MSTATUS64_SXL, MXL_RV64);
-mstatus = set_field(mstatus, MSTATUS64_UXL, MXL_RV64);
}
env->mstatus = mstatus;
Still missing the update for write_sstatus, which I think is simply an update to
sstatus_v1_10_mask.
r~
Re: [RFC PATCH v2 01/30] target/loongarch: Update README
Hi, Xiaojuan, On Thu, Nov 11, 2021 at 9:41 AM Xiaojuan Yang wrote: > > Mainly introduce how to run the softmmu > > Signed-off-by: Xiaojuan Yang > Signed-off-by: Song Gao > --- > target/loongarch/README | 20 > 1 file changed, 20 insertions(+) > > diff --git a/target/loongarch/README b/target/loongarch/README > index 09f809cf80..6f64bde22f 100644 > --- a/target/loongarch/README > +++ b/target/loongarch/README > @@ -71,6 +71,26 @@ >./qemu-loongarch64 /opt/clfs/usr/bin/pwd >... > > +- Softmmu emulation > + > + Add support softmmu emulation support in the following series patches. > + Mainly emulate a virt 3A5000 board that is not exactly the same as the > host. > + Kernel code is on the github and the uefi code will be opened in the near > future. > + All required binaries can get from github for test. > + > + 1.Download kernel and the cross-tools.(vmlinux) > + > + wget https://github.com/loongson/linux This is a git repo URL, I think we cannot use wget to download. > + wget > https://github.com/loongson/build-tools/releases/latest/download/loongarch64-clfs-20210831-cross-tools.tar.xz > + > + 2.Download the clfs-system and made a ramdisk with busybox.(ramdisk) > + > + 3.Run with command,eg: > + > + ./build/qemu-system-loongarch64 -m 4G -smp 16 --cpu Loongson-3A5000 > --machine loongson7a -kernel ./vmlinux -initrd ./ramdisk -append > "root=/dev/ram console=ttyS0,115200 rdinit=/sbin/init loglevel=8" -monitor > tcp::4000,server,nowait -nographic It isn't recommended to use "loongson7a" as the machine name. In my opinion, we will have two types of machines run in qemu (One is an emulated LS7A and the other is pure virtual). I think we can call them "loongson3-ls7a" and "loongson3-virt". Huacai > + > +The vmlinux, ramdisk and uefi binary loongarch_bios.bin can get from : > +git clone https://github.com/yangxiaojuan-loongson/qemu-binary > > - Note. >We can get the latest LoongArch documents or LoongArch tools at > https://github.com/loongson/ > -- > 2.27.0 > > -- Huacai Chen
[PATCH v2 00/10] block: Attempt on fixing 030-reported errors
Hi,
v1 cover letter:
https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg01287.html
In v2 I’ve addressed the comments I’ve received from Kevin and Vladimir.
To this end, I’ve retained only the non-controversial part in patch 5,
and split everything else (i.e. the stuff relating to
bdrv_replace_child_tran()) into the dedicated patches 6 and 8.
Kevin’s most important comments (to my understanding) were that:
(A) When bdrv_remove_file_or_backing_child() uses
bdrv_replace_child_tran(), we have to ensure that the BDS lives as
long as the transaction does. This is solved by keeping a strong
reference to it that’s released only when the transaction is cleaned
up (and the new patch 7 ensures that the .clean() handler will be
invoked after all .commit()/.abort() handlers, so the reference will
really live as long as the transaction does).
(B) bdrv_replace_node_noperm() passes a pointer to loop-local variable,
which is a really bad idea considering the transaction lives much
longer than one loop iteration.
Luckily, the new_bs it passes is always non-NULL, and so
bdrv_replace_child_tran() actually doesn’t need to store the
BdrvChild ** pointer, because for a non-NULL new_bs, there is
nothing to revert in the abort handler. We just need to clarify
this, not store the pointer in case of a non-NULL new_bs, and assert
that bdrv_replace_node_noperm() and its relatives are only used to
replace an existing node by some other existing (i.e. non-NULL)
node.
git-backport-diff against v1:
Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/10:[] [--] 'stream: Traverse graph after modification'
002/10:[0005] [FC] 'block: Manipulate children list in .attach/.detach'
003/10:[] [--] 'block: Unite remove_empty_child and child_free'
004/10:[] [--] 'block: Drop detached child from ignore list'
005/10:[0040] [FC] 'block: Pass BdrvChild ** to replace_child_noperm'
006/10:[down] 'block: Restructure remove_file_or_backing_child()'
007/10:[down] 'transactions: Invoke clean() after everything else'
008/10:[down] 'block: Let replace_child_tran keep indirect pointer'
009/10:[0020] [FC] 'block: Let replace_child_noperm free children'
010/10:[] [--] 'iotests/030: Unthrottle parallel jobs in reverse'
Detailed per-patch changes:
Patch 2:
- Dropped stale comment about undoing bdrv_attach_child_noperm()’s list
insertion in the respective abort handler
Patch 5:
- Split off everything related to bdrv_replace_child_tran()
Patch 6:
- Added (split off from patch 5)
- Keep the `if (!child) { return; }` path before getting childp
Patch 7:
- Added: I wanted bdrv_remove_file_or_backing_child() to keep a strong
BDS reference throughout the transaction, and the simplest way (i.e.
the one where I wouldn’t have to think too hard) was to add this
patch and have transactions invoke .clean() after all
.commit()/.abort() handlers are done. This way we can just drop the
reference in .clean() and need not care about the order the
transaction actions were added in.
Patch 8:
- Added (split off from patch 5)
- The BdrvChild ** pointer is now only stored in the
BdrvReplaceChildState if new_bs is NULL. Otherwise, we don’t need
it, because we won’t need to reinstate the child in the abort
handler. This way we don’t have to worry about
bdrv_replace_node_noperm() passing a pointer to a loop-local
variable (because the new_bs it passes is never NULL).
- In the same vein, assert that bdrv_replace_node() and relatives
cannot be called to replace the node by NULL.
- Have bdrv_remove_file_or_backing_child() keep a strong reference to
the parent BDS throughout the transaction, so the &bs->backing or
&bs->file pointer remains valid
- Moved the comment explaining why we want to pass &s->child instead of
s->childp to bdrv_replace_child_noperm() in
bdrv_replace_child_abort() from patch 9 here (and also keep passing
&s->child instead of s->childp). It is already relevant now that
s->childp is valid only if new_bs is NULL.
Patch 9:
- The comment this used to add to bdrv_replace_child_abort() is now
already added by patch 8, we just need to drop the TODO note there
- Also drop the TODO note above bdrv_replace_child_tran()
Hanna Reitz (10):
stream: Traverse graph after modification
block: Manipulate children list in .attach/.detach
block: Unite remove_empty_child and child_free
block: Drop detached child from ignore list
block: Pass BdrvChild ** to replace_child_noperm
block: Restructure remove_file_or_backing_child()
transactions: Invoke clean() after everything else
block: Let replace_child_tran keep indirect pointer
block: Let replace_child_noperm free children
iotests/030: Unthrottle parallel jobs in reverse
[PATCH v2 01/10] stream: Traverse graph after modification
bdrv_cor_filter_drop() modifies the block graph. That means that other
parties can also modify the block graph before it returns. Therefore,
we cannot assume that the result of a graph traversal we did before
remains valid afterwards.
We should thus fetch `base` and `unfiltered_base` afterwards instead of
before.
Signed-off-by: Hanna Reitz
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/stream.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 97bee482dc..e45113aed6 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -54,8 +54,8 @@ static int stream_prepare(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
-BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
-BlockDriverState *unfiltered_base = bdrv_skip_filters(base);
+BlockDriverState *base;
+BlockDriverState *unfiltered_base;
Error *local_err = NULL;
int ret = 0;
@@ -63,6 +63,9 @@ static int stream_prepare(Job *job)
bdrv_cor_filter_drop(s->cor_filter_bs);
s->cor_filter_bs = NULL;
+base = bdrv_filter_or_cow_bs(s->above_base);
+unfiltered_base = bdrv_skip_filters(base);
+
if (bdrv_cow_child(unfiltered_bs)) {
const char *base_id = NULL, *base_fmt = NULL;
if (unfiltered_base) {
--
2.33.1
[PATCH v2 03/10] block: Unite remove_empty_child and child_free
Now that bdrv_remove_empty_child() no longer removes the child from the
parent's children list but only checks that it is not in such a list, it
is only a wrapper around bdrv_child_free() that checks that the child is
empty and unused. That should apply to all children that we free, so
put those checks into bdrv_child_free() and drop
bdrv_remove_empty_child().
Signed-off-by: Hanna Reitz
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/block.c b/block.c
index ca024ffced..19bff4f95c 100644
--- a/block.c
+++ b/block.c
@@ -2740,19 +2740,19 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
}
}
-static void bdrv_child_free(void *opaque)
-{
-BdrvChild *c = opaque;
-
-g_free(c->name);
-g_free(c);
-}
-
-static void bdrv_remove_empty_child(BdrvChild *child)
+/**
+ * Free the given @child.
+ *
+ * The child must be empty (i.e. `child->bs == NULL`) and it must be
+ * unused (i.e. not in a children list).
+ */
+static void bdrv_child_free(BdrvChild *child)
{
assert(!child->bs);
assert(!child->next.le_prev); /* not in children list */
-bdrv_child_free(child);
+
+g_free(child->name);
+g_free(child);
}
typedef struct BdrvAttachChildCommonState {
@@ -2786,7 +2786,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
}
bdrv_unref(bs);
-bdrv_remove_empty_child(child);
+bdrv_child_free(child);
*s->child = NULL;
}
@@ -2859,7 +2859,7 @@ static int bdrv_attach_child_common(BlockDriverState
*child_bs,
if (ret < 0) {
error_propagate(errp, local_err);
-bdrv_remove_empty_child(new_child);
+bdrv_child_free(new_child);
return ret;
}
}
@@ -2925,7 +2925,7 @@ static void bdrv_detach_child(BdrvChild *child)
BlockDriverState *old_bs = child->bs;
bdrv_replace_child_noperm(child, NULL);
-bdrv_remove_empty_child(child);
+bdrv_child_free(child);
if (old_bs) {
/*
--
2.33.1
[PATCH v2 02/10] block: Manipulate children list in .attach/.detach
The children list is specific to BDS parents. We should not modify it
in the general children modification code, but let BDS parents deal with
it in their .attach() and .detach() methods.
This also has the advantage that a BdrvChild is removed from the
children list before its .bs pointer can become NULL. BDS parents
generally assume that their children's .bs pointer is never NULL, so
this is actually a bug fix.
Signed-off-by: Hanna Reitz
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/block.c b/block.c
index 580cb77a70..ca024ffced 100644
--- a/block.c
+++ b/block.c
@@ -1387,6 +1387,8 @@ static void bdrv_child_cb_attach(BdrvChild *child)
{
BlockDriverState *bs = child->opaque;
+QLIST_INSERT_HEAD(&bs->children, child, next);
+
if (child->role & BDRV_CHILD_COW) {
bdrv_backing_attach(child);
}
@@ -1403,6 +1405,8 @@ static void bdrv_child_cb_detach(BdrvChild *child)
}
bdrv_unapply_subtree_drain(child, bs);
+
+QLIST_REMOVE(child, next);
}
static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
@@ -2747,7 +2751,7 @@ static void bdrv_child_free(void *opaque)
static void bdrv_remove_empty_child(BdrvChild *child)
{
assert(!child->bs);
-QLIST_SAFE_REMOVE(child, next);
+assert(!child->next.le_prev); /* not in children list */
bdrv_child_free(child);
}
@@ -2913,12 +2917,6 @@ static int bdrv_attach_child_noperm(BlockDriverState
*parent_bs,
return ret;
}
-QLIST_INSERT_HEAD(&parent_bs->children, *child, next);
-/*
- * child is removed in bdrv_attach_child_common_abort(), so don't care to
- * abort this change separately.
- */
-
return 0;
}
@@ -4851,7 +4849,6 @@ static void bdrv_remove_filter_or_cow_child_abort(void
*opaque)
BdrvRemoveFilterOrCowChild *s = opaque;
BlockDriverState *parent_bs = s->child->opaque;
-QLIST_INSERT_HEAD(&parent_bs->children, s->child, next);
if (s->is_backing) {
parent_bs->backing = s->child;
} else {
@@ -4906,7 +4903,6 @@ static void
bdrv_remove_file_or_backing_child(BlockDriverState *bs,
};
tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
-QLIST_SAFE_REMOVE(child, next);
if (s->is_backing) {
bs->backing = NULL;
} else {
--
2.33.1
