Re: [RFC PATCH 02/23] kvm: Switch KVM_CAP_READONLY_MEM to a per-VM ioctl()

[Philippe Mathieu-Daudé]

Hi Isaku,

On 2/16/21 3:12 AM, Isaku Yamahata wrote:
> Switch to making a VM ioctl() call for KVM_CAP_READONLY_MEM, which may
> be conditional on VM type in recent versions of KVM, e.g. when TDX is
> supported.
> 
> Signed-off-by: Isaku Yamahata 
> ---
>  accel/kvm/kvm-all.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 47516913b7..351c25a5cb 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2164,7 +2164,7 @@ static int kvm_init(MachineState *ms)
>  }
>  
>  kvm_readonly_mem_allowed =
> -(kvm_check_extension(s, KVM_CAP_READONLY_MEM) > 0);
> +(kvm_vm_check_extension(s, KVM_CAP_READONLY_MEM) > 0);

Can this check with "recent KVM" be a problem with older ones?

Maybe for backward compatibility we need:

  = (kvm_vm_check_extension(s, KVM_CAP_READONLY_MEM) > 0) ||
(kvm_check_extension(s, KVM_CAP_READONLY_MEM) > 0);

>  kvm_eventfds_allowed =
>  (kvm_check_extension(s, KVM_CAP_IOEVENTFD) > 0);
> 

Re: [RFC PATCH 01/23] target/i386: Expose x86_cpu_get_supported_feature_word() for TDX

[Philippe Mathieu-Daudé]

On 2/16/21 3:12 AM, Isaku Yamahata wrote:
> From: Sean Christopherson 
> 
> Expose x86_cpu_get_supported_feature_word() outside of cpu.c so that it
> can be used by TDX to setup the VM-wide CPUID configuration.
> 
> Signed-off-by: Sean Christopherson 
> ---
>  target/i386/cpu.c | 4 ++--
>  target/i386/cpu.h | 3 +++
>  2 files changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] virtio-pci: add check for vdev in virtio_pci_isr_read

[Philippe Mathieu-Daudé]

Hi Yuri,

On 2/16/21 6:29 AM, Yuri Benditovich wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1743098

Maybe add backtrace in patch description?

(gdb) bt
#0  0xc5bbdf0d in virtio_pci_notify_write (opaque=0x55b6c6dff170,
addr=0, val=<...>, size=<...>)
at hw/virtio/virtio-pci.c:1360
#1  0xc59fe596 in memory_region_write_accessor (mr=<...>, addr=<...>,
value=<...>, size=<...>, shift=<...>, mask=<...>, attrs=...)
at memory.c:530
#2  0xc59fc9e6 in access_with_adjusted_size (addr=addr@entry=0,
value=..., size=size@entry=2, access_size_min=<...>,
access_size_max=<...>, access_fn=,
mr=0x55b6c6df7cd0, attrs=...)
at memory.c:597
#3  0xc5a0084a in memory_region_dispatch_write (mr=0x55b6c6df7cd0,
addr=0, data=<...>, size=2, attrs=...)
at memory.c:1474
#4  0xc59aebbc in flatview_write (fv=0x7f1abc0407c0, addr=<...>,
attrs=..., buf=<...>, len=<...>)
at exec.c:3099
#5  0xc59b3243 in address_space_write (as=<...>, addr=<...>, attrs=...,
buf=<...>, len=<...>)
at exec.c:3265
#6  0xc5a0f878 in kvm_cpu_exec (cpu=<...>)
at accel/kvm/kvm-all.c:2004
#7  0xc59ec21e in qemu_kvm_cpu_thread_fn (arg=0x55b6c6b0)
at cpus.c:1215
#8  0x7f1adb4732de in start_thread (arg=<...>) at pthread_create.c:486
#9  0x7f1adb1a4133 in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:95

> There is missing check for vdev in this procedure.
> QEMU crash happens in it in hot unplug flow.
> 
> Signed-off-by: Yuri Benditovich 
> ---
>  hw/virtio/virtio-pci.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 094c36aa3e..2f19301267 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1364,7 +1364,13 @@ static uint64_t virtio_pci_isr_read(void *opaque, 
> hwaddr addr,
>  {
>  VirtIOPCIProxy *proxy = opaque;
>  VirtIODevice *vdev = virtio_bus_get_device(>bus);
> -uint64_t val = qatomic_xchg(>isr, 0);
> +uint64_t val = 0;

   uint64_t val;

> +
> +if (vdev == NULL) {
> +return val;

   return 0;

> +}
> +
> +val = qatomic_xchg(>isr, 0);
>  pci_irq_deassert(>pci_dev);
>  
>  return val;
> 

This pattern is present in other functions of this file.

Can you fix them too, or add an assert() if it is unlikely
to happen?

Thanks,

Phil.

Re: [PATCH v2 04/42] esp: add vmstate_esp version to embedded ESPState

[Philippe Mathieu-Daudé]

On 2/9/21 8:29 PM, Mark Cave-Ayland wrote:
> The QOM object representing ESPState is currently embedded within both the
> SYSBUS_ESP and PCI_ESP devices with migration state handled by embedding
> vmstate_esp within each device using VMSTATE_STRUCT.
> 
> Since the vmstate_esp fields are embedded directly within the migration
> stream, the incoming vmstate_esp version_id is lost. The only version 
> information
> available is that from vmstate_sysbus_esp_scsi and vmstate_esp_pci_scsi, but
> those versions represent their respective devices and not that of the 
> underlying
> ESPState.
> 
> Resolve this by adding a new version-dependent field in 
> vmstate_sysbus_esp_scsi
> and vmstate_esp_pci_scsi which stores the vmstate_esp version_id field within
> ESPState to be used to allow migration from older QEMU versions.
> 
> Finally bump the vmstate_esp version to 5 to cover the upcoming ESPState 
> changes
> within this patch series.
> 
> Signed-off-by: Mark Cave-Ayland 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/scsi/esp-pci.c |  3 ++-
>  hw/scsi/esp.c | 23 +--
>  include/hw/scsi/esp.h |  2 ++
>  3 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index ccf73bb901..8a82404853 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -330,11 +330,12 @@ static void esp_pci_hard_reset(DeviceState *dev)
>  
>  static const VMStateDescription vmstate_esp_pci_scsi = {
>  .name = "pciespscsi",
> -.version_id = 1,
> +.version_id = 2,
>  .minimum_version_id = 1,
>  .fields = (VMStateField[]) {
>  VMSTATE_PCI_DEVICE(parent_obj, PCIESPState),
>  VMSTATE_BUFFER_UNSAFE(dma_regs, PCIESPState, 0, 8 * 
> sizeof(uint32_t)),
> +VMSTATE_UINT8_V(esp.mig_version_id, PCIESPState, 2),
>  VMSTATE_STRUCT(esp, PCIESPState, 0, vmstate_esp, ESPState),
>  VMSTATE_END_OF_LIST()
>  }
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 1635f86622..9427c55d1d 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -796,10 +796,28 @@ static const VMStateDescription vmstate_esp_pdma = {
>  }
>  };
>  
> +static int esp_pre_save(void *opaque)
> +{
> +ESPState *s = ESP(opaque);
> +
> +s->mig_version_id = vmstate_esp.version_id;
> +return 0;
> +}
> +
> +static int esp_post_load(void *opaque, int version_id)
> +{
> +ESPState *s = ESP(opaque);
> +
> +s->mig_version_id = vmstate_esp.version_id;
> +return 0;
> +}
> +
>  const VMStateDescription vmstate_esp = {
>  .name = "esp",
> -.version_id = 4,
> +.version_id = 5,
>  .minimum_version_id = 3,
> +.pre_save = esp_pre_save,
> +.post_load = esp_post_load,
>  .fields = (VMStateField[]) {
>  VMSTATE_BUFFER(rregs, ESPState),
>  VMSTATE_BUFFER(wregs, ESPState),
> @@ -996,9 +1014,10 @@ static void sysbus_esp_init(Object *obj)
>  
>  static const VMStateDescription vmstate_sysbus_esp_scsi = {
>  .name = "sysbusespscsi",
> -.version_id = 1,
> +.version_id = 2,
>  .minimum_version_id = 1,
>  .fields = (VMStateField[]) {
> +VMSTATE_UINT8_V(esp.mig_version_id, SysBusESPState, 2),
>  VMSTATE_STRUCT(esp, SysBusESPState, 0, vmstate_esp, ESPState),
>  VMSTATE_END_OF_LIST()
>  }
> diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
> index 11c799d91e..7d92471c5b 100644
> --- a/include/hw/scsi/esp.h
> +++ b/include/hw/scsi/esp.h
> @@ -68,6 +68,8 @@ struct ESPState {
>  uint32_t pdma_start;
>  uint32_t pdma_cur;
>  void (*pdma_cb)(ESPState *s);
> +
> +uint8_t mig_version_id;
>  };
>  
>  #define TYPE_SYSBUS_ESP "sysbus-esp"
> 

Re: [PATCH v2 08/42] esp: determine transfer direction directly from SCSI phase

[Philippe Mathieu-Daudé]

On 2/9/21 8:29 PM, Mark Cave-Ayland wrote:
> The transfer direction is currently determined by checking the sign of ti_size
> but as this series progresses ti_size can be zero at the end of the transfer.
> 
> Use the SCSI phase to determine the transfer direction as used in other SCSI
> controller implementations.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/scsi/esp.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 11/42] esp: apply transfer length adjustment when STC is zero at TC load time

[Philippe Mathieu-Daudé]

On 2/9/21 8:29 PM, Mark Cave-Ayland wrote:
> Perform the length adjustment whereby a value of 0 in the STC represents
> a transfer length of 0x1 at the point where the TC is loaded at the

0x1 -> 64 KiB?

> start of a DMA command rather than just when a TI (Transfer Information)
> command is executed. This better matches the description as given in the
> datasheet.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/scsi/esp.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index a1acc2c9bd..02b7876394 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -562,9 +562,6 @@ static void handle_ti(ESPState *s)
>  }
>  
>  dmalen = esp_get_tc(s);
> -if (dmalen == 0) {
> -dmalen = 0x1;
> -}
>  s->dma_counter = dmalen;
>  
>  if (s->do_cmd) {
> @@ -699,7 +696,11 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t 
> val)
>  if (val & CMD_DMA) {
>  s->dma = 1;
>  /* Reload DMA counter.  */
> -esp_set_tc(s, esp_get_stc(s));
> +if (esp_get_stc(s) == 0) {
> +esp_set_tc(s, 0x1);

0x1 -> 64 * KiB

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 20/42] esp: remove the buf and buflen parameters from get_cmd()

[Philippe Mathieu-Daudé]

On 2/9/21 8:29 PM, Mark Cave-Ayland wrote:
> Now that all SCSI commands are accumulated in cmdbuf, remove the buf and 
> buflen
> parameters from get_cmd() since these always reference cmdbuf and 
> ESP_CMDBUF_SZ
> respectively.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/scsi/esp.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 30/42] esp: add 4 byte PDMA read and write transfers

[Philippe Mathieu-Daudé]

Hi Mark,

On 2/15/21 11:35 PM, Mark Cave-Ayland wrote:
> On 12/02/2021 18:56, Philippe Mathieu-Daudé wrote:
> 
>> On 2/9/21 8:30 PM, Mark Cave-Ayland wrote:
>>> The MacOS toolbox ROM performs 4 byte reads/writes when transferring
>>> data to
>>> and from the target. Since the SCSI bus is 16-bits wide, use the
>>> memory API
>>> to split a 4 byte access into 2 x 2 byte accesses.
>>>
>>> Signed-off-by: Mark Cave-Ayland 
>>> ---
>>>   hw/scsi/esp.c | 6 --
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> Reviewed-by: Philippe Mathieu-Daudé 
>>
>> Out of curiosity, what is the bus used?
> 
> AFAICT it's a custom logic chip that sits on the CPU address/data buses
> that does the decoding between the CPU and ESP chip. Other than a simple
> block diagram of the Quadra there isn't much official documentation out
> there :/

OK.

> Are you planning to review any more of this series? I'm keen to put out
> a (hopefully final) v3 soon, but I'll hold off for little while if you
> want more time to look over the remaining patches.

I talked about this series with Laurent on Sunday, asking him for
review help ;) I don't remember if there is any big comment to
address in patches 1-14. If not I can review the missing ones
there today and you could send directly a pull request for this
first set, then send the rest as v3. Does that help?
For the rest I doubt having time to focus before Friday.

Regards,

Phil.

Re: [PATCH] scripts/checkpatch: Improve the check for authors mangled by the mailing list

[Philippe Mathieu-Daudé]

On 2/16/21 8:15 AM, Thomas Huth wrote:
> There were recently some patches on the list which had their "From:"
> line mangled like this:
> 
>  From: qemu_oss--- via 
> 
> Since our test in the checkpatch.pl script did not trigger here, the
> patches finally also ended up in a pull request, with the wrong author
> set. So let's improve the regular expression to also complain on
> these new patterns, too.

:(

> 
> Signed-off-by: Thomas Huth 
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 

[PATCH] scripts/checkpatch: Improve the check for authors mangled by the mailing list

[Thomas Huth]

There were recently some patches on the list which had their "From:"
line mangled like this:

 From: qemu_oss--- via 

Since our test in the checkpatch.pl script did not trigger here, the
patches finally also ended up in a pull request, with the wrong author
set. So let's improve the regular expression to also complain on
these new patterns, too.

Signed-off-by: Thomas Huth 
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e47ad878d8..7f194c842b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1499,7 +1499,7 @@ sub process {
$is_patch = 1;
}
 
-   if ($line =~ /^Author: .*via 
Qemu-devel.*/) {
+   if ($line =~ /^(Author|From): .* via 
.*/) {
ERROR("Author email address is mangled by the mailing 
list\n" . $herecurr);
}
 
-- 
2.27.0

Re: [RFC PATCH 28/42] target/mips/tx79: Move RDHWR usermode kludge to trans_SQ()

[Fredrik Noring]

On Mon, Feb 15, 2021 at 01:01:52PM -0800, Richard Henderson wrote:
> On 2/14/21 9:58 AM, Philippe Mathieu-Daudé wrote:
> > +/*
> > + * The TX79-specific instruction Store Quadword
> > + *
> > + * ++---+---++
> > + * | 01 |  base |   rt  |   offset   | SQ
> > + * ++---+---++
> > + *  6   5   5 16
> > + *
> > + * has the same opcode as the Read Hardware Register instruction
> > + *
> > + * ++---+---+---+---++
> > + * | 01 | 0 |   rt  |   rd  | 0 | 111011 | RDHWR
> > + * ++---+---+---+---++
> > + *  6   5   5   5   56
> > + *
> > + * that is required, trapped and emulated by the Linux kernel. 
> > However, all
> > + * RDHWR encodings yield address error exceptions on the TX79 since 
> > the SQ
> > + * offset is odd.
> 
> Not that it's odd (the final address is masked, remember), but that it a store
> to an address in the zero page.

The address always resolves to 0xe83b (then masked) in 32-bit KSEG2,
because rt is always $3 and rd is always $29 so -6085(zero), hence the
last page (which is much better) rather than the first, as Maciej
discovered:

https://patchwork.kernel.org/comment/23824173/

Other possible RDHWR encodings are no longer used, and can therefore be
ignored and revert to SQ:

https://patchwork.kernel.org/comment/23842167/

Notice also the oddity of 32-bit R5900 addresses that doesn't matter here
but has implications for n32 and system emulation.

> I would do this as
> 
> {
>   RDHWR_user  01 0 . . 0 111011   @rd_rt
>   SQ  01 . .  @ldst
> }

Both rd and rt have fixed values, as mentioned.

For reference, RDHWR is currently done like this in the Linux kernel:

if (IS_ENABLED(CONFIG_CPU_R5900)) {
/*
 * On the R5900, a valid RDHWR instruction
 *
 * ++---+++---++
 * | 01 | 0 | rt | rd | 0 | 111011 |
 * ++---+++---++
 *  6   5  55 56
 *
 * having rt $3 (v1) and rd $29 (MIPS_HWR_ULR) is
 * interpreted as the R5900 specific SQ instruction
 *
 * ++---++-+
 * | 01 |  base | rt |offset   |
 * ++---++-+
 *  6   5  516
 *
 * with
 *
 * sq v1,-6085(zero)
 *
 * that asserts an address exception since -6085(zero)
 * always resolves to 0xe83b in 32-bit KSEG2.
 *
 * Other legacy values of rd, such as MIPS_HWR_CPUNUM,
 * are ignored.
 */
if (insn.r_format.func == rdhwr_op &&
insn.r_format.rd == MIPS_HWR_ULR &&
insn.r_format.rt == 3 &&
insn.r_format.rs == 0 &&
insn.r_format.re == 0) {
if (compute_return_epc(regs) < 0 ||
simulate_rdhwr(regs, insn.r_format.rd,
   insn.r_format.rt) < 0)
goto sigill;
return;
}
goto sigbus;
} else ...

Fredrik

Re: [PATCH] gitlab-ci: Only push Docker 'latest' image when building default branch

[Philippe Mathieu-Daudé]

On 2/16/21 7:55 AM, Thomas Huth wrote:
> On 15/02/2021 20.28, Philippe Mathieu-Daudé wrote:
>> While we are interested in building docker images in different
>> branches, it only makes sense to push 'latest' to the registry
>> when this is the project default branch (usually 'master').
>>
>> Else when pushing different branches concurrently we might have
>> inconsistent image state between branches.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>   .gitlab-ci.d/containers.yml | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/.gitlab-ci.d/containers.yml b/.gitlab-ci.d/containers.yml
>> index 90fac85ce46..52a915f4141 100644
>> --- a/.gitlab-ci.d/containers.yml
>> +++ b/.gitlab-ci.d/containers.yml
>> @@ -17,7 +17,7 @@
>>     -t "qemu/$NAME" -f "tests/docker/dockerfiles/$NAME.docker"
>>     -r $CI_REGISTRY_IMAGE
>>   - docker tag "qemu/$NAME" "$TAG"
>> -    - docker push "$TAG"
>> +    - test "$CI_COMMIT_BRANCH" = "$CI_DEFAULT_BRANCH" && docker push
>> "$TAG"
> 
> So does that mean that the following stages in the CI (i.e. build, test)
> are only always (i.e. also for the non-master branches) going to use
> containers that have been build on the master branch?

Hmm good point. Should we use "$CI_COMMIT_BRANCH" instead of "latest"?

Re: [PATCH] gitlab-ci: Only push Docker 'latest' image when building default branch

[Thomas Huth]

On 15/02/2021 20.28, Philippe Mathieu-Daudé wrote:

While we are interested in building docker images in different
branches, it only makes sense to push 'latest' to the registry
when this is the project default branch (usually 'master').

Else when pushing different branches concurrently we might have
inconsistent image state between branches.

Signed-off-by: Philippe Mathieu-Daudé 
---
  .gitlab-ci.d/containers.yml | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitlab-ci.d/containers.yml b/.gitlab-ci.d/containers.yml
index 90fac85ce46..52a915f4141 100644
--- a/.gitlab-ci.d/containers.yml
+++ b/.gitlab-ci.d/containers.yml
@@ -17,7 +17,7 @@
-t "qemu/$NAME" -f "tests/docker/dockerfiles/$NAME.docker"
-r $CI_REGISTRY_IMAGE
  - docker tag "qemu/$NAME" "$TAG"
-- docker push "$TAG"
+- test "$CI_COMMIT_BRANCH" = "$CI_DEFAULT_BRANCH" && docker push "$TAG"


So does that mean that the following stages in the CI (i.e. build, test) are 
only always (i.e. also for the non-master branches) going to use containers 
that have been build on the master branch?


 Thomas

[PATCH] virtio-pci: add check for vdev in virtio_pci_isr_read

[Yuri Benditovich]

https://bugzilla.redhat.com/show_bug.cgi?id=1743098
There is missing check for vdev in this procedure.
QEMU crash happens in it in hot unplug flow.

Signed-off-by: Yuri Benditovich 
---
 hw/virtio/virtio-pci.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 094c36aa3e..2f19301267 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1364,7 +1364,13 @@ static uint64_t virtio_pci_isr_read(void *opaque, hwaddr 
addr,
 {
 VirtIOPCIProxy *proxy = opaque;
 VirtIODevice *vdev = virtio_bus_get_device(>bus);
-uint64_t val = qatomic_xchg(>isr, 0);
+uint64_t val = 0;
+
+if (vdev == NULL) {
+return val;
+}
+
+val = qatomic_xchg(>isr, 0);
 pci_irq_deassert(>pci_dev);
 
 return val;
-- 
2.17.1

Re: [PATCH v7 03/14] block: check return value of bdrv_open_child and drop error propagation

[Vladimir Sementsov-Ogievskiy]

15.02.2021 23:04, Eric Blake wrote:

On 2/15/21 3:22 AM, Kevin Wolf wrote:


With this patch applied, 'check unit-test' fails with:

Running test test-replication
Unexpected error in bdrv_open_driver() at ../block.c:1481:
Could not open '/tmp/p_local_disk.z1Ugyc': Invalid argument
ERROR test-replication - missing test plan




The problem is this hunk:

diff --git a/block/qcow2.c b/block/qcow2.c
index 5d94f45be9..e8dd42d73b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1611,9 +1611,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
  /* Open external data file */
  s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
 _of_bds, BDRV_CHILD_DATA,
-   true, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+   true, errp);
+if (!s->data_file) {
  ret = -EINVAL;
  goto fail;
  }

bdrv_open_child() can return NULL in non-error cases, when the child is
optional and it isn't given. The test doesn't use an external data file,
so this returns NULL without setting an error, which now gets turned
into -EINVAL instead.

This makes the most basic tests fail with qcow2 (iotests 001 is enough).

The other hunks in this patch don't suffer from the same problem because
they pass allow_none=false.


Thanks; that's enough to figure out how to repair the patch:

diff --git i/block/qcow2.c w/block/qcow2.c
index e8dd42d73b4c..38137ca30eb0 100644
--- i/block/qcow2.c
+++ w/block/qcow2.c
@@ -1292,6 +1292,7 @@ static int
validate_compression_type(BDRVQcow2State *s, Error **errp)
  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
  {
+ERRP_GUARD();
  BDRVQcow2State *s = bs->opaque;
  unsigned int len, i;
  int ret = 0;
@@ -1612,7 +1613,7 @@ static int coroutine_fn
qcow2_do_open(BlockDriverState *bs, QDict *options,
  s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
 _of_bds, BDRV_CHILD_DATA,
 true, errp);
-if (!s->data_file) {
+if (*errp) {
  ret = -EINVAL;
  goto fail;
  }




Agree.. Or better refactor bdrv_open_child to follow more common (and 
recommended) semantics (i.e. NULL + errp on error, non-null on succsess).. But 
this will require more investigation.

I'm busy now with our internal deadline (this week), after this will return to 
handle upstream things.

--
Best regards,
Vladimir

[PULL 7/9] Acceptance Tests: introduce method for requiring an accelerator

[Cleber Rosa]

Some tests explicitly require a QEMU accelerator to be available.
Given that this depends on some runtime aspects not known before
the test is started, such as the currently set QEMU binary, it's
left to be checked also at runtime.

Signed-off-by: Cleber Rosa 
Message-Id: <20210203172357.1422425-17-cr...@redhat.com>
Reviewed-by: Beraldo Leal 
Reviewed-by: Willian Rampazzo 
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/avocado_qemu/__init__.py | 24 
 tests/acceptance/boot_linux.py| 34 ++-
 tests/acceptance/virtiofs_submounts.py|  5 +---
 3 files changed, 34 insertions(+), 29 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index b06692a59df..687c5dc0cf6 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -39,6 +39,8 @@
 
 sys.path.append(os.path.join(SOURCE_DIR, 'python'))
 
+from qemu.accel import kvm_available
+from qemu.accel import tcg_available
 from qemu.machine import QEMUMachine
 
 def is_readable_executable_file(path):
@@ -162,6 +164,28 @@ def _get_unique_tag_val(self, tag_name):
 return vals.pop()
 return None
 
+def require_accelerator(self, accelerator):
+"""
+Requires an accelerator to be available for the test to continue
+
+It takes into account the currently set qemu binary.
+
+If the check fails, the test is canceled.  If the check itself
+for the given accelerator is not available, the test is also
+canceled.
+
+:param accelerator: name of the accelerator, such as "kvm" or "tcg"
+:type accelerator: str
+"""
+checker = {'tcg': tcg_available,
+   'kvm': kvm_available}.get(accelerator)
+if checker is None:
+self.cancel("Don't know how to check for the presence "
+"of accelerator %s" % accelerator)
+if not checker(qemu_bin=self.qemu_bin):
+self.cancel("%s accelerator does not seem to be "
+"available" % accelerator)
+
 def setUp(self):
 self._vms = {}
 
diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index 14e89d020d1..0d178038a09 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -12,15 +12,8 @@
 
 from avocado_qemu import LinuxTest, BUILD_DIR
 
-from qemu.accel import kvm_available
-from qemu.accel import tcg_available
-
 from avocado import skipIf
 
-ACCEL_NOT_AVAILABLE_FMT = "%s accelerator does not seem to be available"
-KVM_NOT_AVAILABLE = ACCEL_NOT_AVAILABLE_FMT % "KVM"
-TCG_NOT_AVAILABLE = ACCEL_NOT_AVAILABLE_FMT % "TCG"
-
 
 class BootLinuxX8664(LinuxTest):
 """
@@ -34,8 +27,7 @@ def test_pc_i440fx_tcg(self):
 :avocado: tags=machine:pc
 :avocado: tags=accel:tcg
 """
-if not tcg_available(self.qemu_bin):
-self.cancel(TCG_NOT_AVAILABLE)
+self.require_accelerator("tcg")
 self.vm.add_args("-accel", "tcg")
 self.launch_and_wait()
 
@@ -44,8 +36,7 @@ def test_pc_i440fx_kvm(self):
 :avocado: tags=machine:pc
 :avocado: tags=accel:kvm
 """
-if not kvm_available(self.arch, self.qemu_bin):
-self.cancel(KVM_NOT_AVAILABLE)
+self.require_accelerator("kvm")
 self.vm.add_args("-accel", "kvm")
 self.launch_and_wait()
 
@@ -54,8 +45,7 @@ def test_pc_q35_tcg(self):
 :avocado: tags=machine:q35
 :avocado: tags=accel:tcg
 """
-if not tcg_available(self.qemu_bin):
-self.cancel(TCG_NOT_AVAILABLE)
+self.require_accelerator("tcg")
 self.vm.add_args("-accel", "tcg")
 self.launch_and_wait()
 
@@ -64,8 +54,7 @@ def test_pc_q35_kvm(self):
 :avocado: tags=machine:q35
 :avocado: tags=accel:kvm
 """
-if not kvm_available(self.arch, self.qemu_bin):
-self.cancel(KVM_NOT_AVAILABLE)
+self.require_accelerator("kvm")
 self.vm.add_args("-accel", "kvm")
 self.launch_and_wait()
 
@@ -91,8 +80,7 @@ def test_virt_tcg(self):
 :avocado: tags=accel:tcg
 :avocado: tags=cpu:max
 """
-if not tcg_available(self.qemu_bin):
-self.cancel(TCG_NOT_AVAILABLE)
+self.require_accelerator("tcg")
 self.vm.add_args("-accel", "tcg")
 self.vm.add_args("-cpu", "max")
 self.vm.add_args("-machine", "virt,gic-version=2")
@@ -105,8 +93,7 @@ def test_virt_kvm_gicv2(self):
 :avocado: tags=cpu:host
 :avocado: tags=device:gicv2
 """
-if not kvm_available(self.arch, self.qemu_bin):
-self.cancel(KVM_NOT_AVAILABLE)
+self.require_accelerator("kvm")
 self.vm.add_args("-accel", "kvm")
 self.vm.add_args("-cpu", "host")
 self.vm.add_args("-machine", "virt,gic-version=2")
@@ -119,8 +106,7 @@ def 

[PULL 9/9] Acceptance Tests: set up existing ssh keys by default

[Cleber Rosa]

It's questionable whether it's necessary to create one brand new pair
for each test.  It's not questionable that it takes less time and
resources to just use the keys available at "tests/keys" that exist
for that exact reason.

If a location for the public key is not given explicitly, the
LinuxTest will now set up the existing pair of keys as the default.
This removes the need for a lot of boilerplate code.

To avoid the ssh client from erroring on permission issues, a
directory with restrictive permissions is created for the private key.
This should still be a lot cheaper than creating a new key.

Signed-off-by: Cleber Rosa 
Message-Id: <20210203172357.1422425-19-cr...@redhat.com>
Reviewed-by: Willian Rampazzo 
Reviewed-by: Marc-André Lureau 
[marcandre: fix typos in commit message]
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/avocado_qemu/__init__.py | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index 51e9055c986..df167b142cc 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -10,6 +10,7 @@
 
 import logging
 import os
+import shutil
 import sys
 import uuid
 import tempfile
@@ -254,8 +255,21 @@ def setUp(self, ssh_pubkey=None):
 self.vm.add_args('-smp', '2')
 self.vm.add_args('-m', '1024')
 self.set_up_boot()
+if ssh_pubkey is None:
+ssh_pubkey, self.ssh_key = self.set_up_existing_ssh_keys()
 self.set_up_cloudinit(ssh_pubkey)
 
+def set_up_existing_ssh_keys(self):
+ssh_public_key = os.path.join(SOURCE_DIR, 'tests', 'keys', 
'id_rsa.pub')
+source_private_key = os.path.join(SOURCE_DIR, 'tests', 'keys', 
'id_rsa')
+ssh_dir = os.path.join(self.workdir, '.ssh')
+os.mkdir(ssh_dir, mode=0o700)
+ssh_private_key = os.path.join(ssh_dir,
+   os.path.basename(source_private_key))
+shutil.copyfile(source_private_key, ssh_private_key)
+os.chmod(ssh_private_key, 0o600)
+return (ssh_public_key, ssh_private_key)
+
 def download_boot(self):
 self.log.debug('Looking for and selecting a qemu-img binary to be '
'used to create the bootable snapshot image')
-- 
2.26.2

[PULL 5/9] maint: Tell git that *.py files should use python diff hunks

[Cleber Rosa]

From: Eric Blake 

Git's default hunk pattern recognizer favors the C language, but it
also includes several built-in diff styles that give saner results in
other languages.  In particular, telling git to treat all .py files as
python changes the beginning of diff hunks as follows:

|  --- a/python/qemu/machine.py
|  +++ b/python/qemu/machine.py
| -@@ -337,12 +337,12 @@ class QEMUMachine:
| +@@ -337,12 +337,12 @@ def _post_shutdown(self) -> None:
|   self._qmp.close()

which makes it much easier to tell what function a patch is touching,
rather than a non-descript listing of what class contains the changes.

Sadly, our python files that don't use .py suffix (such as numerous
iotests) do not benefit from this glob.

Reported-by: John Snow 
Signed-off-by: Eric Blake 
Message-Id: <20210215222524.1820223-1-ebl...@redhat.com>
Reviewed-by: John Snow 
Reviewed-by: Cleber Rosa 
Signed-off-by: Cleber Rosa 
---
 .gitattributes | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitattributes b/.gitattributes
index 3d2fe2ecda8..07f430e9441 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -1,2 +1,3 @@
 *.c.inc diff=c
 *.h.inc diff=c
+*.pydiff=python
-- 
2.26.2

[PULL 8/9] Acceptance Tests: fix population of public key in cloudinit image

[Cleber Rosa]

Currently the path of the ssh public key is being set, but its
content is obviously what's needed.

Signed-off-by: Cleber Rosa 
Message-Id: <20210203172357.1422425-18-cr...@redhat.com>
Reviewed-by: Marc-André Lureau 
Reviewed-by: Wainer dos Santos Moschetta 
Reviewed-by: Willian Rampazzo 
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/avocado_qemu/__init__.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index 687c5dc0cf6..51e9055c986 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -290,13 +290,15 @@ def prepare_cloudinit(self, ssh_pubkey=None):
 try:
 cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
 self.phone_home_port = network.find_free_port()
+with open(ssh_pubkey) as pubkey:
+pubkey_content = pubkey.read()
 cloudinit.iso(cloudinit_iso, self.name,
   username='root',
   password='password',
   # QEMU's hard coded usermode router address
   phone_home_host='10.0.2.2',
   phone_home_port=self.phone_home_port,
-  authorized_key=ssh_pubkey)
+  authorized_key=pubkey_content)
 except Exception:
 self.cancel('Failed to prepare the cloudinit image')
 return cloudinit_iso
-- 
2.26.2

[PULL 6/9] Acceptance Tests: introduce LinuxTest base class

[Cleber Rosa]

This is basically the infrastructure around "boot_linux.py" tests, but
now made into a base class for general use.

Signed-off-by: Cleber Rosa 
Message-Id: <20210203172357.1422425-15-cr...@redhat.com>
Reviewed-by: Wainer dos Santos Moschetta 
Reviewed-by: Willian Rampazzo 
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/avocado_qemu/__init__.py | 87 +
 tests/acceptance/boot_linux.py| 94 ++-
 tests/acceptance/virtiofs_submounts.py|  6 +-
 3 files changed, 94 insertions(+), 93 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index bf54e419da2..b06692a59df 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -16,6 +16,13 @@
 
 import avocado
 
+from avocado.utils import cloudinit
+from avocado.utils import datadrainer
+from avocado.utils import network
+from avocado.utils import vmimage
+from avocado.utils.path import find_command
+
+
 #: The QEMU build root directory.  It may also be the source directory
 #: if building from the source dir, but it's safer to use BUILD_DIR for
 #: that purpose.  Be aware that if this code is moved outside of a source
@@ -206,3 +213,83 @@ def fetch_asset(self, name,
 expire=expire,
 find_only=find_only,
 cancel_on_missing=cancel_on_missing)
+
+
+class LinuxTest(Test):
+"""Facilitates having a cloud-image Linux based available.
+
+For tests that indend to interact with guests, this is a better choice
+to start with than the more vanilla `Test` class.
+"""
+
+timeout = 900
+chksum = None
+
+def setUp(self, ssh_pubkey=None):
+super(LinuxTest, self).setUp()
+self.vm.add_args('-smp', '2')
+self.vm.add_args('-m', '1024')
+self.set_up_boot()
+self.set_up_cloudinit(ssh_pubkey)
+
+def download_boot(self):
+self.log.debug('Looking for and selecting a qemu-img binary to be '
+   'used to create the bootable snapshot image')
+# If qemu-img has been built, use it, otherwise the system wide one
+# will be used.  If none is available, the test will cancel.
+qemu_img = os.path.join(BUILD_DIR, 'qemu-img')
+if not os.path.exists(qemu_img):
+qemu_img = find_command('qemu-img', False)
+if qemu_img is False:
+self.cancel('Could not find "qemu-img", which is required to '
+'create the bootable image')
+vmimage.QEMU_IMG = qemu_img
+
+self.log.info('Downloading/preparing boot image')
+# Fedora 31 only provides ppc64le images
+image_arch = self.arch
+if image_arch == 'ppc64':
+image_arch = 'ppc64le'
+try:
+boot = vmimage.get(
+'fedora', arch=image_arch, version='31',
+checksum=self.chksum,
+algorithm='sha256',
+cache_dir=self.cache_dirs[0],
+snapshot_dir=self.workdir)
+except:
+self.cancel('Failed to download/prepare boot image')
+return boot.path
+
+def prepare_cloudinit(self, ssh_pubkey=None):
+self.log.info('Preparing cloudinit image')
+try:
+cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
+self.phone_home_port = network.find_free_port()
+cloudinit.iso(cloudinit_iso, self.name,
+  username='root',
+  password='password',
+  # QEMU's hard coded usermode router address
+  phone_home_host='10.0.2.2',
+  phone_home_port=self.phone_home_port,
+  authorized_key=ssh_pubkey)
+except Exception:
+self.cancel('Failed to prepare the cloudinit image')
+return cloudinit_iso
+
+def set_up_boot(self):
+path = self.download_boot()
+self.vm.add_args('-drive', 'file=%s' % path)
+
+def set_up_cloudinit(self, ssh_pubkey=None):
+cloudinit_iso = self.prepare_cloudinit(ssh_pubkey)
+self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
+
+def launch_and_wait(self):
+self.vm.set_console()
+self.vm.launch()
+console_drainer = 
datadrainer.LineLogger(self.vm.console_socket.fileno(),
+ 
logger=self.log.getChild('console'))
+console_drainer.start()
+self.log.info('VM launched, waiting for boot confirmation from guest')
+cloudinit.wait_for_phone_home(('0.0.0.0', self.phone_home_port), 
self.name)
diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index bcd923bb4a0..14e89d020d1 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -10,16 +10,11 @@
 
 import os
 
-from avocado_qemu 

[PULL 1/9] Acceptance Tests: bump Avocado version requirement to 85.0

[Cleber Rosa]

This version (and 84.0) contain improvements that address specific
QEMU use cases, including:

 * Being able to download and use Fedora 31 images and thus
   re-activate the "boot_linux.py" tests

 * Being able to register local assets via "avocado assets register"
   and use them in tests

Signed-off-by: Cleber Rosa 
Message-Id: <20210211232835.2608059-2-cr...@redhat.com>
Acked-by: Philippe Mathieu-Daudé 
Reviewed-by: Wainer dos Santos Moschetta 
Reviewed-by: Willian Rampazzo 
Signed-off-by: Cleber Rosa 
---
 tests/requirements.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/requirements.txt b/tests/requirements.txt
index 62e8ffd28c2..91f3a343b95 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -1,5 +1,5 @@
 # Add Python module requirements, one per line, to be installed
 # in the tests/venv Python virtual environment. For more info,
 # refer to: https://pip.pypa.io/en/stable/user_guide/#id1
-avocado-framework==83.0
+avocado-framework==85.0
 pycdlib==1.11.0
-- 
2.26.2

[PULL 4/9] tests/acceptance/virtio-gpu.py: preserve virtio-user-gpu log

[Cleber Rosa]

Preserve log at location already prepared for keeping the test's log
files.

While at it, log info about its location (in the main test log
file), instead of printing it out.

Reference: 
https://avocado-framework.readthedocs.io/en/85.0/api/test/avocado.html#avocado.Test.logdir
Signed-off-by: Cleber Rosa 
Reviewed-by: Wainer dos Santos Moschetta 
Reviewed-by: Philippe Mathieu-Daudé 
[philmd: use full sentence]
Message-Id: <20210211220146.2525771-7-cr...@redhat.com>

Signed-off-by: Cleber Rosa 
---
 tests/acceptance/virtio-gpu.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/virtio-gpu.py b/tests/acceptance/virtio-gpu.py
index 211f02932f2..ab1a4c1a718 100644
--- a/tests/acceptance/virtio-gpu.py
+++ b/tests/acceptance/virtio-gpu.py
@@ -119,10 +119,11 @@ def test_vhost_user_vga_virgl(self):
 os.set_inheritable(vug_sock.fileno(), True)
 
 self._vug_log_path = os.path.join(
-self.vm._test_dir, "vhost-user-gpu.log"
+self.logdir, "vhost-user-gpu.log"
 )
 self._vug_log_file = open(self._vug_log_path, "wb")
-print(self._vug_log_path)
+self.log.info('Complete vhost-user-gpu.log file can be '
+  'found at %s', self._vug_log_path)
 
 vugp = subprocess.Popen(
 [vug, "--virgl", "--fd=%d" % vug_sock.fileno()],
-- 
2.26.2

[PULL 3/9] Python: close the log file kept by QEMUMachine before reading it

[Cleber Rosa]

Closing a file that is open for writing, and then reading from it
sounds like a better idea than the opposite, given that the content
will be flushed.

Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
Signed-off-by: Cleber Rosa 
Message-Id: <20210211220146.2525771-2-cr...@redhat.com>
Reviewed-by: John Snow 
Signed-off-by: Cleber Rosa 
---
 python/qemu/machine.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 7a40f4604be..6e44bda337e 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -337,12 +337,12 @@ def _post_shutdown(self) -> None:
 self._qmp.close()
 self._qmp_connection = None
 
-self._load_io_log()
-
 if self._qemu_log_file is not None:
 self._qemu_log_file.close()
 self._qemu_log_file = None
 
+self._load_io_log()
+
 self._qemu_log_path = None
 
 if self._temp_dir is not None:
-- 
2.26.2

[PULL 2/9] virtiofs_submounts.py test: Note on vmlinuz param

[Cleber Rosa]

From: Max Reitz 

>From the cancel message, it is not entirely clear why this parameter is
mandatory now, or that it will be optional in the future.  Add such a
more detailed explanation as a comment in the test source file.

Suggested-by: Alex Bennée 
Signed-off-by: Max Reitz 
Message-Id: <20210212151649.252440-1-mre...@redhat.com>
Reviewed-by: Cleber Rosa 
Reviewed-by: Willian Rampazzo 
Reviewed-by: Alex Bennée 
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/virtiofs_submounts.py | 12 
 1 file changed, 12 insertions(+)

diff --git a/tests/acceptance/virtiofs_submounts.py 
b/tests/acceptance/virtiofs_submounts.py
index 949ca87a837..9a69b6b17bc 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -228,6 +228,18 @@ def live_cleanup(self):
 def setUp(self):
 vmlinuz = self.params.get('vmlinuz')
 if vmlinuz is None:
+"""
+The Linux kernel supports FUSE auto-submounts only as of 5.10.
+boot_linux.py currently provides Fedora 31, whose kernel is too
+old, so this test cannot pass with the on-image kernel (you are
+welcome to try, hence the option to force such a test with
+-p vmlinuz='').  Therefore, for now the user must provide a
+sufficiently new custom kernel, or effectively explicitly
+request failure with -p vmlinuz=''.
+Once an image with a sufficiently new kernel is available
+(probably Fedora 34), we can make -p vmlinuz='' the default, so
+that this parameter no longer needs to be specified.
+"""
 self.cancel('vmlinuz parameter not set; you must point it to a '
 'Linux kernel binary to test (to run this test with ' \
 'the on-image kernel, set it to an empty string)')
-- 
2.26.2

[PULL 0/9] Acceptance Tests and Python libs patches for 2021-02-15

[Cleber Rosa]

The following changes since commit 8ba4bca570ace1e60614a0808631a517cf5df67a:

  Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging=
 (2021-02-15 17:13:57 +)

are available in the Git repository at:

  https://gitlab.com/cleber.gnu/qemu.git/ tags/python-next-pull-request

for you to fetch changes up to c0c5a7f18e623b8f6eb7a9ccebdccdc56db2cec7:

  Acceptance Tests: set up existing ssh keys by default (2021-02-15 22:30:06 =
-0500)


Acceptance Tests and Python libs improvements

Along with the Acceptance Tests and Python libs improvements, a
improvement to the diff generation for Python code.



Cleber Rosa (7):
  Acceptance Tests: bump Avocado version requirement to 85.0
  Python: close the log file kept by QEMUMachine before reading it
  tests/acceptance/virtio-gpu.py: preserve virtio-user-gpu log
  Acceptance Tests: introduce LinuxTest base class
  Acceptance Tests: introduce method for requiring an accelerator
  Acceptance Tests: fix population of public key in cloudinit image
  Acceptance Tests: set up existing ssh keys by default

Eric Blake (1):
  maint: Tell git that *.py files should use python diff hunks

Max Reitz (1):
  virtiofs_submounts.py test: Note on vmlinuz param

 .gitattributes|   1 +
 python/qemu/machine.py|   4 +-
 tests/acceptance/avocado_qemu/__init__.py | 127 +
 tests/acceptance/boot_linux.py| 128 +++---
 tests/acceptance/virtio-gpu.py|   5 +-
 tests/acceptance/virtiofs_submounts.py|  23 ++--
 tests/requirements.txt|   2 +-
 7 files changed, 163 insertions(+), 127 deletions(-)

--=20
2.26.2

[PATCH v2 6/6] hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed

[Bin Meng]

If the block size is programmed to a different value from the
previous one, reset the data pointer of s->fifo_buffer[] so that
s->fifo_buffer[] can be filled in using the new block size in
the next transfer.

With this fix, the following reproducer:

outl 0xcf8 0x80001010
outl 0xcfc 0xe000
outl 0xcf8 0x80001001
outl 0xcfc 0x0600
write 0xe02c 0x1 0x05
write 0xe005 0x1 0x02
write 0xe007 0x1 0x01
write 0xe028 0x1 0x10
write 0x0 0x1 0x23
write 0x2 0x1 0x08
write 0xe00c 0x1 0x01
write 0xe00e 0x1 0x20
write 0xe00f 0x1 0x00
write 0xe00c 0x1 0x32
write 0xe004 0x2 0x0200
write 0xe028 0x1 0x00
write 0xe003 0x1 0x40

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -machine accel=qtest -m 512M \
  -nodefaults -device sdhci-pci,sd-spec-version=3 \
  -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
  -device sd-card,drive=mydrive -qtest stdio

Cc: qemu-sta...@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov 
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng 

---

Changes in v2:
- new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a different 
block size is programmed

 hw/sd/sdhci.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index d0c8e29..5b86781 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1140,6 +1140,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 break;
 case SDHC_BLKSIZE:
 if (!TRANSFERRING_DATA(s->prnsts)) {
+uint16_t blksize = s->blksize;
+
 MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
 MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
 
@@ -1151,6 +1153,16 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 
 s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
 }
+
+/*
+ * If the block size is programmed to a different value from
+ * the previous one, reset the data pointer of s->fifo_buffer[]
+ * so that s->fifo_buffer[] can be filled in using the new block
+ * size in the next transfer.
+ */
+if (blksize != s->blksize) {
+s->data_count = 0;
+}
 }
 
 break;
-- 
2.7.4

[PATCH v2 5/6] hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is writable

[Bin Meng]

The codes to limit the maximum block size is only necessary when
SDHC_BLKSIZE register is writable.

Signed-off-by: Bin Meng 

---

Changes in v2:
- new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable

 hw/sd/sdhci.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 7a2003b..d0c8e29 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1142,15 +1142,15 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 if (!TRANSFERRING_DATA(s->prnsts)) {
 MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
 MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
-}
 
-/* Limit block size to the maximum buffer size */
-if (extract32(s->blksize, 0, 12) > s->buf_maxsz) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than "
-  "the maximum buffer 0x%x\n", __func__, s->blksize,
-  s->buf_maxsz);
+/* Limit block size to the maximum buffer size */
+if (extract32(s->blksize, 0, 12) > s->buf_maxsz) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than "
+  "the maximum buffer 0x%x\n", __func__, 
s->blksize,
+  s->buf_maxsz);
 
-s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
+s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
+}
 }
 
 break;
-- 
2.7.4

[PATCH v2 3/6] hw/sd: sdhci: Correctly set the controller status for ADMA

[Bin Meng]

When an ADMA transfer is started, the codes forget to set the
controller status to indicate a transfer is in progress.

With this fix, the following 2 reproducers:

https://paste.debian.net/plain/1185136
https://paste.debian.net/plain/1185141

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -machine accel=qtest -m 512M \
  -nodefaults -device sdhci-pci,sd-spec-version=3 \
  -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
  -device sd-card,drive=mydrive -qtest stdio

Cc: qemu-sta...@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov 
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng 
---

(no changes since v1)

 hw/sd/sdhci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 05cb281..0b0ca6f 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -769,7 +769,9 @@ static void sdhci_do_adma(SDHCIState *s)
 
 switch (dscr.attr & SDHC_ADMA_ATTR_ACT_MASK) {
 case SDHC_ADMA_ATTR_ACT_TRAN:  /* data transfer */
+s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
 if (s->trnmod & SDHC_TRNS_READ) {
+s->prnsts |= SDHC_DOING_READ;
 while (length) {
 if (s->data_count == 0) {
 sdbus_read_data(>sdbus, s->fifo_buffer, block_size);
@@ -797,6 +799,7 @@ static void sdhci_do_adma(SDHCIState *s)
 }
 }
 } else {
+s->prnsts |= SDHC_DOING_WRITE;
 while (length) {
 begin = s->data_count;
 if ((length + begin) < block_size) {
-- 
2.7.4

[PATCH v2 4/6] hw/sd: sdhci: Simplify updating s->prnsts in sdhci_sdma_transfer_multi_blocks()

[Bin Meng]

s->prnsts is updated in both branches of the if () else () statement.
Move the common bits outside so that it is cleaner.

Signed-off-by: Bin Meng 
---

(no changes since v1)

 hw/sd/sdhci.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 0b0ca6f..7a2003b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -598,9 +598,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 page_aligned = true;
 }
 
+s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
 if (s->trnmod & SDHC_TRNS_READ) {
-s->prnsts |= SDHC_DOING_READ | SDHC_DATA_INHIBIT |
-SDHC_DAT_LINE_ACTIVE;
+s->prnsts |= SDHC_DOING_READ;
 while (s->blkcnt) {
 if (s->data_count == 0) {
 sdbus_read_data(>sdbus, s->fifo_buffer, block_size);
@@ -627,8 +627,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 }
 }
 } else {
-s->prnsts |= SDHC_DOING_WRITE | SDHC_DATA_INHIBIT |
-SDHC_DAT_LINE_ACTIVE;
+s->prnsts |= SDHC_DOING_WRITE;
 while (s->blkcnt) {
 begin = s->data_count;
 if (((boundary_count + begin) < block_size) && page_aligned) {
-- 
2.7.4

[PATCH v2 2/6] hw/sd: sdhci: Don't write to SDHC_SYSAD register when transfer is in progress

[Bin Meng]

Per "SD Host Controller Standard Specification Version 7.00"
chapter 2.2.1 SDMA System Address Register:

This register can be accessed only if no transaction is executing
(i.e., after a transaction has stopped).

With this fix, the following reproducer:

https://paste.debian.net/plain/1185137

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -machine accel=qtest -m 512M \
   -nodefaults -device sdhci-pci,sd-spec-version=3 \
   -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
   -device sd-card,drive=mydrive -qtest stdio

Cc: qemu-sta...@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov 
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng 
---

(no changes since v1)

 hw/sd/sdhci.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 1c5ab26..05cb281 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1122,15 +1122,17 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 
 switch (offset & ~0x3) {
 case SDHC_SYSAD:
-s->sdmasysad = (s->sdmasysad & mask) | value;
-MASKED_WRITE(s->sdmasysad, mask, value);
-/* Writing to last byte of sdmasysad might trigger transfer */
-if (!(mask & 0xFF00) && TRANSFERRING_DATA(s->prnsts) && s->blkcnt 
&&
-s->blksize && SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
-if (s->trnmod & SDHC_TRNS_MULTI) {
-sdhci_sdma_transfer_multi_blocks(s);
-} else {
-sdhci_sdma_transfer_single_block(s);
+if (!TRANSFERRING_DATA(s->prnsts)) {
+s->sdmasysad = (s->sdmasysad & mask) | value;
+MASKED_WRITE(s->sdmasysad, mask, value);
+/* Writing to last byte of sdmasysad might trigger transfer */
+if (!(mask & 0xFF00) && s->blkcnt && s->blksize &&
+SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
+if (s->trnmod & SDHC_TRNS_MULTI) {
+sdhci_sdma_transfer_multi_blocks(s);
+} else {
+sdhci_sdma_transfer_single_block(s);
+}
 }
 }
 break;
-- 
2.7.4

[PATCH v2 1/6] hw/sd: sdhci: Don't transfer any data when command time out

[Bin Meng]

At the end of sdhci_send_command(), it starts a data transfer if the
command register indicates data is associated. But the data transfer
should only be initiated when the command execution has succeeded.

With this fix, the following reproducer:

outl 0xcf8 0x80001810
outl 0xcfc 0xe1068000
outl 0xcf8 0x80001804
outw 0xcfc 0x7
write 0xe106802c 0x1 0x0f
write 0xe1068004 0xc 0x2801d10101fbff28a384
write 0xe106800c 0x1f 
0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
write 0xe1068003 0x28 
0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
write 0xe1068003 0x1 0xfe

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -M pc-q35-5.0 \
  -device sdhci-pci,sd-spec-version=3 \
  -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
  -device sd-card,drive=mydrive \
  -monitor none -serial none -qtest stdio

Cc: qemu-sta...@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov 
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng 
Acked-by: Alistair Francis 
Tested-by: Alexander Bulekov 
Tested-by: Philippe Mathieu-Daudé 
---

(no changes since v1)

 hw/sd/sdhci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 8ffa539..1c5ab26 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -326,6 +326,7 @@ static void sdhci_send_command(SDHCIState *s)
 SDRequest request;
 uint8_t response[16];
 int rlen;
+bool timeout = false;
 
 s->errintsts = 0;
 s->acmd12errsts = 0;
@@ -349,6 +350,7 @@ static void sdhci_send_command(SDHCIState *s)
 trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
s->rspreg[1], s->rspreg[0]);
 } else {
+timeout = true;
 trace_sdhci_error("timeout waiting for command response");
 if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
 s->errintsts |= SDHC_EIS_CMDTIMEOUT;
@@ -369,7 +371,7 @@ static void sdhci_send_command(SDHCIState *s)
 
 sdhci_update_irq(s);
 
-if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
+if (!timeout && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
 s->data_count = 0;
 sdhci_data_transfer(s);
 }
-- 
2.7.4

[PATCH v2 0/6] hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409

[Bin Meng]

This series includes several fixes to CVE-2020-17380, CVE-2020-25085
and CVE-2021-3409 that are heap-based buffer overflow issues existing
in the sdhci model.

These CVEs are pretty much similar, and were filed using different
reproducers. With this series, current known reproducers I have
cannot be reproduced any more.

The implementation of this model still has some issues, e.g.: some codes
do not seem to strictly match the spec, but since this series only aimes
to address CVEs, such issue should be fixed in a separate series in the
future, if I have time :)

Changes in v2:
- new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable
- new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a different 
block size is programmed

Bin Meng (6):
  hw/sd: sdhci: Don't transfer any data when command time out
  hw/sd: sdhci: Don't write to SDHC_SYSAD register when transfer is in
progress
  hw/sd: sdhci: Correctly set the controller status for ADMA
  hw/sd: sdhci: Simplify updating s->prnsts in
sdhci_sdma_transfer_multi_blocks()
  hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is
writable
  hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a
different block size is programmed

 hw/sd/sdhci.c | 60 ++-
 1 file changed, 39 insertions(+), 21 deletions(-)

-- 
2.7.4

Re: [PATCH 00/22] Acceptance Test: introduce base class for Linux based tests

[Cleber Rosa]

On Mon, Feb 15, 2021 at 06:03:33PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/15/21 4:49 PM, Wainer dos Santos Moschetta wrote:
> > Hi,
> > 
> > On 2/8/21 8:35 AM, Philippe Mathieu-Daudé wrote:
> >> On 2/3/21 6:23 PM, Cleber Rosa wrote:
> >>> This introduces a base class for tests that need to interact with a
> >>> Linux guest.  It generalizes the "boot_linux.py" code, already been
> >>> used by the "virtiofs_submounts.py" and also SSH related code being
> >>> used by that and "linux_ssh_mips_malta.py".
> >>>
> >>> While at it, a number of fixes on hopeful improvements to those tests
> >>> were added.
> >>>
> >>> Cleber Rosa (22):
> >>>    tests/acceptance/boot_linux.py: fix typo on cloudinit error message
> >>>    tests/acceptance/boot_linux.py: rename misleading cloudinit method
> >>>    Acceptance Tests: remove unnecessary tag from documentation example
> >>>    tests/acceptance/virtiofs_submounts.py: use workdir property
> >>>    tests/acceptance/virtiofs_submounts.py: do not ask for ssh key
> >>>  password
> >>>    tests/acceptance/virtiofs_submounts.py: use a virtio-net device
> >>>  instead
> >>>    tests/acceptance/virtiofs_submounts.py: evaluate string not length
> >>>    tests/acceptance/virtiofs_submounts.py: standardize port as integer
> >>>    tests/acceptance/virtiofs_submounts.py: required space between IP and
> >>>  port
> >>>    Python: add utility function for retrieving port redirection
> >>>    tests/acceptance/linux_ssh_mips_malta.py: standardize port as integer
> >>>    Acceptance tests: clarify ssh connection failure reason
> >>>    tests/acceptance/virtiofs_submounts.py: add missing accel tag
> >>>    Acceptance Tests: introduce LinuxTest base class
> >>>    Acceptance Tests: move useful ssh methods to base class
> >>>    Acceptance Tests: introduce method for requiring an accelerator
> >>>    Acceptance Tests: fix population of public key in cloudinit image
> >>>    Acceptance Tests: set up existing ssh keys by default
> >>>    Acceptance Tests: add port redirection for ssh by default
> >>>    Acceptance Tests: add basic documentation on LinuxTest base class
> >>>    Acceptance Tests: introduce CPU hotplug test
> >>>    [NOTFORMERGE] Bump Avocado version to latest master
> >>>
> >>>   docs/devel/testing.rst    |  29 +++-
> >>>   python/qemu/utils.py  |  35 +
> >>>   tests/acceptance/avocado_qemu/__init__.py | 176 ++
> >>>   tests/acceptance/boot_linux.py    | 128 ++--
> >>>   tests/acceptance/hotplug_cpu.py   |  38 +
> >>>   tests/acceptance/info_usernet.py  |  29 
> >>>   tests/acceptance/linux_ssh_mips_malta.py  |  44 +-
> >>>   tests/acceptance/virtiofs_submounts.py    |  73 +
> >>>   tests/requirements.txt    |   2 +-
> >>>   tests/vm/basevm.py    |   7 +-
> >>>   10 files changed, 334 insertions(+), 227 deletions(-)
> >>>   create mode 100644 python/qemu/utils.py
> >>>   create mode 100644 tests/acceptance/hotplug_cpu.py
> >>>   create mode 100644 tests/acceptance/info_usernet.py
> >> Patches 1-6, 8-9 & 12 queued.
> > 
> > 
> > Those are merged. Most of the remaining patches got at least one review,
> > so could you (Cleber or Philippe) open a pull request for them as well?
> > Telling it because there are many series in flight for the acceptance
> > tests, and to avoid conflicts with future series...
> 
> I asked a question to Cleber in patch 13 and am waiting what he meant
> before queuing the series (fixing the typo Marc-André noticed).
> 
> Regards,
> 
> Phil.
> 

Hi Phil,

Thanks for taking the previous patches, and thanks for the willingness
to take (and tweak) some more.  I only saw this comment now, and I
have a bunch of these ready to fire a PR.

I guess I also provided an answer to the question you're referring to.

If my tree passes CI, then I should be sending a PR within the next
few hours.

Best,
- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH 18/22] Acceptance Tests: set up existing ssh keys by default

[Cleber Rosa]

On Thu, Feb 11, 2021 at 02:15:32PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Feb 3, 2021 at 10:07 PM Cleber Rosa  wrote:
> >
> > It's questionable wether it's necessary to create one brand new pair
> 
> whether
>

Yep, thanks!

> > for each test.  It's not questionable that it takes less time and
> > resources to just use the keys available at "tests/keys" that exist
> > for that exact reason.
> >
> > If a location for the public key is not given explicitly, the
> > LinuxTest will now set up the existing pair of keys as the default.
> > This removes the need for a lot of boiler plate code.
> 
> boilerplate
>

You're right again.

> >
> > To avoid the ssh client from erroring on permission issues, a
> > directory with restricive permissions is created for the private key.
> 
> restrictive
>

OK.



> > This should still be a lot cheaper than creating a new key.
> >
> > Signed-off-by: Cleber Rosa 
> 
> Reviewed-by: Marc-André Lureau 
>

Thanks for the review,
- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH 14/22] Acceptance Tests: introduce LinuxTest base class

[Cleber Rosa]

On Mon, Feb 15, 2021 at 04:06:45PM -0300, Willian Rampazzo wrote:
> On Wed, Feb 3, 2021 at 2:24 PM Cleber Rosa  wrote:
> >
> > This is basically the infrastructure around "boot_linux.py" tests, but
> > now made into a base class for general use.
> >
> > Signed-off-by: Cleber Rosa 
> > ---
> >  tests/acceptance/avocado_qemu/__init__.py | 87 +
> >  tests/acceptance/boot_linux.py| 94 ++-
> >  tests/acceptance/virtiofs_submounts.py|  6 +-
> >  3 files changed, 94 insertions(+), 93 deletions(-)
> >
> > diff --git a/tests/acceptance/avocado_qemu/__init__.py 
> > b/tests/acceptance/avocado_qemu/__init__.py
> > index bf54e419da..b06692a59d 100644
> > --- a/tests/acceptance/avocado_qemu/__init__.py
> > +++ b/tests/acceptance/avocado_qemu/__init__.py
> 
> I found it not so intuitive to have the base class defined here. I see
> the number of base classes for the tests growing in the future. A
> common place for base classes for tests would be better, just like the
> `qemu.utils` you are defining somewhere else. Anyway, this is a design
> decision that can be changed later, so
>

Hi Willian,

I tend to agree, and my medium/long term vision is similar.  What I
expect to be able to do soon (this is connected to John's work) is to
have "avocado_qemu" as something like "qemu.testing.functional" which
describe its Avocado (and other) dependencies.

A "qemu.test.other" could describe its own dependencies (which may
not include Avocado).

> Reviewed-by: Willian Rampazzo 
> 
> 

Thanks for the review,
- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH] maint: Tell git that *.py files should use python diff hunks

[Cleber Rosa]

On Mon, Feb 15, 2021 at 04:25:24PM -0600, Eric Blake wrote:
> Git's default hunk pattern recognizer favors the C language, but it
> also includes several built-in diff styles that give saner results in
> other languages.  In particular, telling git to treat all .py files as
> python changes the beginning of diff hunks as follows:
> 
> |  --- a/python/qemu/machine.py
> |  +++ b/python/qemu/machine.py
> | -@@ -337,12 +337,12 @@ class QEMUMachine:
> | +@@ -337,12 +337,12 @@ def _post_shutdown(self) -> None:
> |   self._qmp.close()
> 
> which makes it much easier to tell what function a patch is touching,
> rather than a non-descript listing of what class contains the changes.
> 
> Sadly, our python files that don't use .py suffix (such as numerous
> iotests) do not benefit from this glob.
> 
> Reported-by: John Snow 
> Signed-off-by: Eric Blake 
> ---
>  .gitattributes | 1 +
>  1 file changed, 1 insertion(+)
>

Cool stuff!  I'm queueing this, if you don't mind.

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

[PATCH v6 19/19] qapi/introspect.py: add SchemaMetaType enum

[John Snow]

Follows the qapi/introspect.py definition of the same; this adds a more
precise typing to _gen_tree's mtype parameter.

NB: print(SchemaMetaType.BUILTIN) would produce the string
"SchemaMetaType.BUILTIN", but when using format strings (.format or f-strings),
it relies on the __format__ method defined in the Enum class, which uses the
"value" of the enum instead, producing the string "builtin".

For consistency with old-style format strings (which simply call the
__str__ method of an object), a __str__ dunder is added, though it is
not actually used here in this code.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 38 +-
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index c6f5cf8d874..008a21f5c4c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -11,6 +11,7 @@
 See the COPYING file in the top-level directory.
 """
 
+from enum import Enum
 from typing import (
 Any,
 Dict,
@@ -79,6 +80,23 @@
 SchemaInfoCommand = Dict[str, object]
 
 
+class SchemaMetaType(str, Enum):
+"""
+Mimics the SchemaMetaType enum from qapi/introspect.json.
+"""
+BUILTIN = 'builtin'
+ENUM = 'enum'
+ARRAY = 'array'
+OBJECT = 'object'
+ALTERNATE = 'alternate'
+COMMAND = 'command'
+EVENT = 'event'
+
+def __str__(self) -> str:
+# Needed for intuitive behavior with old-style format strings.
+return str(self.value)
+
+
 _ValueT = TypeVar('_ValueT', bound=_Value)
 
 
@@ -251,7 +269,8 @@ def _gen_features(features: Sequence[QAPISchemaFeature]
   ) -> List[Annotated[str]]:
 return [Annotated(f.name, f.ifcond) for f in features]
 
-def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
+def _gen_tree(self, name: str, mtype: SchemaMetaType,
+  obj: Dict[str, object],
   ifcond: Sequence[str] = (),
   features: Sequence[QAPISchemaFeature] = ()) -> None:
 """
@@ -299,7 +318,7 @@ def _gen_variant(self, variant: QAPISchemaVariant
 
 def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
json_type: str) -> None:
-self._gen_tree(name, 'builtin', {'json-type': json_type})
+self._gen_tree(name, SchemaMetaType.BUILTIN, {'json-type': json_type})
 
 def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
 ifcond: Sequence[str],
@@ -307,7 +326,7 @@ def visit_enum_type(self, name: str, info: 
Optional[QAPISourceInfo],
 members: List[QAPISchemaEnumMember],
 prefix: Optional[str]) -> None:
 self._gen_tree(
-name, 'enum',
+name, SchemaMetaType.ENUM,
 {'values': [Annotated(m.name, m.ifcond) for m in members]},
 ifcond, features
 )
@@ -316,8 +335,8 @@ def visit_array_type(self, name: str, info: 
Optional[QAPISourceInfo],
  ifcond: Sequence[str],
  element_type: QAPISchemaType) -> None:
 element = self._use_type(element_type)
-self._gen_tree('[' + element + ']', 'array', {'element-type': element},
-   ifcond)
+self._gen_tree('[' + element + ']', SchemaMetaType.ARRAY,
+   {'element-type': element}, ifcond)
 
 def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
ifcond: Sequence[str],
@@ -330,14 +349,14 @@ def visit_object_type_flat(self, name: str, info: 
Optional[QAPISourceInfo],
 if variants:
 obj['tag'] = variants.tag_member.name
 obj['variants'] = [self._gen_variant(v) for v in variants.variants]
-self._gen_tree(name, 'object', obj, ifcond, features)
+self._gen_tree(name, SchemaMetaType.OBJECT, obj, ifcond, features)
 
 def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
  ifcond: Sequence[str],
  features: List[QAPISchemaFeature],
  variants: QAPISchemaVariants) -> None:
 self._gen_tree(
-name, 'alternate',
+name, SchemaMetaType.ALTERNATE,
 {'members': [Annotated({'type': self._use_type(m.type)},
m.ifcond)
  for m in variants.variants]},
@@ -361,7 +380,7 @@ def visit_command(self, name: str, info: 
Optional[QAPISourceInfo],
 }
 if allow_oob:
 obj['allow-oob'] = allow_oob
-self._gen_tree(name, 'command', obj, ifcond, features)
+self._gen_tree(name, SchemaMetaType.COMMAND, obj, ifcond, features)
 
 def visit_event(self, name: str, info: Optional[QAPISourceInfo],
 ifcond: Sequence[str], features: List[QAPISchemaFeature],
@@ -370,7 +389,8 @@ 

Re: [PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[Cleber Rosa]

On Mon, Feb 15, 2021 at 03:30:16PM -0300, Wainer dos Santos Moschetta wrote:
> Hi,
> 
> On 2/11/21 7:01 PM, Cleber Rosa wrote:
> > Closing a file that is open for writing, and then reading from it
> > sounds like a better idea than the opposite, given that the content
> > will be flushed.
> > 
> > Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
> > Signed-off-by: Cleber Rosa 
> > ---
> >   python/qemu/machine.py | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > index 7a40f4604b..6e44bda337 100644
> > --- a/python/qemu/machine.py
> > +++ b/python/qemu/machine.py
> > @@ -337,12 +337,12 @@ class QEMUMachine:
> >   self._qmp.close()
> >   self._qmp_connection = None
> > -self._load_io_log()
> > -
> >   if self._qemu_log_file is not None:
> >   self._qemu_log_file.close()
> >   self._qemu_log_file = None
> > +self._load_io_log()
> > +
> 
> 
> IMO it's a too fragile fix. It needs the `self._qemu_log_file.close()` being
> called before `self._load_io_log()` but a future change can make this
> condition unmet again. Maybe you could document that in the code. Or change
> the `_load_io_log()` to close the log file if it is open (also document it
> in code).
> 
> - Wainer
>

I'm glad you see this is needed... and then something else.  I'll investigate
making this more robust as time allows it.

Question is: do you ack/nack this change?

Cheers,
- Cleber.


signature.asc
Description: PGP signature

[PATCH v6 16/19] qapi/introspect.py: Update copyright and authors list

[John Snow]

To reflect the work that went into strictly typing introspect.py,
punish myself by claiming credit.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 5d4f5e23f7e..649225988d1 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -1,10 +1,11 @@
 """
 QAPI introspection generator
 
-Copyright (C) 2015-2018 Red Hat, Inc.
+Copyright (C) 2015-2021 Red Hat, Inc.
 
 Authors:
  Markus Armbruster 
+ John Snow 
 
 This work is licensed under the terms of the GNU GPL, version 2.
 See the COPYING file in the top-level directory.
-- 
2.29.2

[PATCH v6 10/19] qapi/introspect.py: create a typed 'Annotated' data strutcure

[John Snow]

Presently, we use a tuple to attach a dict containing annotations
(comments and compile-time conditionals) to a tree node. This is
undesirable because dicts are difficult to strongly type; promoting it
to a real class allows us to name the values and types of the
annotations we are expecting.

In terms of typing, the Annotated type serves as a generic container
where the annotated node's type is preserved, allowing for greater
specificity than we'd be able to provide without a generic.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 78 ++
 1 file changed, 45 insertions(+), 33 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 3c37c138013..5224be1a333 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -13,8 +13,12 @@
 from typing import (
 Any,
 Dict,
+Generic,
+Iterable,
 List,
 Optional,
+Tuple,
+TypeVar,
 Union,
 )
 
@@ -52,15 +56,25 @@
 _Scalar = Union[str, bool, None]
 _NonScalar = Union[Dict[str, _Stub], List[_Stub]]
 _Value = Union[_Scalar, _NonScalar]
-# JSONValue = TODO, in a forthcoming commit.
+JSONValue = Union[_Value, 'Annotated[_Value]']
 
 
-def _make_tree(obj, ifcond, comment=None):
-extra = {
-'if': ifcond,
-'comment': comment
-}
-return (obj, extra)
+_ValueT = TypeVar('_ValueT', bound=_Value)
+
+
+class Annotated(Generic[_ValueT]):
+"""
+Annotated generally contains a SchemaInfo-like type (as a dict),
+But it also used to wrap comments/ifconds around scalar leaf values,
+for the benefit of features and enums.
+"""
+# TODO: Remove after Python 3.7 adds @dataclass:
+# pylint: disable=too-few-public-methods
+def __init__(self, value: _ValueT, ifcond: Iterable[str],
+ comment: Optional[str] = None):
+self.value = value
+self.comment: Optional[str] = comment
+self.ifcond: Tuple[str, ...] = tuple(ifcond)
 
 
 def _tree_to_qlit(obj, level=0, dict_value=False):
@@ -68,24 +82,20 @@ def _tree_to_qlit(obj, level=0, dict_value=False):
 def indent(level):
 return level * 4 * ' '
 
-if isinstance(obj, tuple):
-ifobj, extra = obj
-ifcond = extra.get('if')
-comment = extra.get('comment')
-
+if isinstance(obj, Annotated):
 # NB: _tree_to_qlit is called recursively on the values of a key:value
 # pair; those values can't be decorated with comments or conditionals.
 msg = "dict values cannot have attached comments or if-conditionals."
 assert not dict_value, msg
 
 ret = ''
-if comment:
-ret += indent(level) + '/* %s */\n' % comment
-if ifcond:
-ret += gen_if(ifcond)
-ret += _tree_to_qlit(ifobj, level)
-if ifcond:
-ret += '\n' + gen_endif(ifcond)
+if obj.comment:
+ret += indent(level) + '/* %s */\n' % obj.comment
+if obj.ifcond:
+ret += gen_if(obj.ifcond)
+ret += _tree_to_qlit(obj.value, level)
+if obj.ifcond:
+ret += '\n' + gen_endif(obj.ifcond)
 return ret
 
 ret = ''
@@ -202,7 +212,7 @@ def _use_type(self, typ):
 
 @staticmethod
 def _gen_features(features):
-return [_make_tree(f.name, f.ifcond) for f in features]
+return [Annotated(f.name, f.ifcond) for f in features]
 
 def _gen_tree(self, name, mtype, obj, ifcond, features):
 comment: Optional[str] = None
@@ -216,7 +226,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
 obj['meta-type'] = mtype
 if features:
 obj['features'] = self._gen_features(features)
-self._trees.append(_make_tree(obj, ifcond, comment))
+self._trees.append(Annotated(obj, ifcond, comment))
 
 def _gen_member(self, member):
 obj = {'name': member.name, 'type': self._use_type(member.type)}
@@ -224,7 +234,7 @@ def _gen_member(self, member):
 obj['default'] = None
 if member.features:
 obj['features'] = self._gen_features(member.features)
-return _make_tree(obj, member.ifcond)
+return Annotated(obj, member.ifcond)
 
 def _gen_variants(self, tag_name, variants):
 return {'tag': tag_name,
@@ -232,16 +242,17 @@ def _gen_variants(self, tag_name, variants):
 
 def _gen_variant(self, variant):
 obj = {'case': variant.name, 'type': self._use_type(variant.type)}
-return _make_tree(obj, variant.ifcond)
+return Annotated(obj, variant.ifcond)
 
 def visit_builtin_type(self, name, info, json_type):
 self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
 
 def visit_enum_type(self, name, info, ifcond, features, members, prefix):
-self._gen_tree(name, 'enum',
-   {'values': [_make_tree(m.name, m.ifcond, None)
-   for m in members]},
-  

[PATCH v6 13/19] qapi/introspect.py: remove _gen_variants helper

[John Snow]

It is easier to give a name to all of the dictly-typed objects we pass
around in introspect.py by removing this helper, as it does not return
an object that has any knowable type by itself.

Inline it into its only caller instead.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index afad891bb2b..b0fcc4443c1 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -241,10 +241,6 @@ def _gen_member(self, member):
 obj['features'] = self._gen_features(member.features)
 return Annotated(obj, member.ifcond)
 
-def _gen_variants(self, tag_name, variants):
-return {'tag': tag_name,
-'variants': [self._gen_variant(v) for v in variants]}
-
 def _gen_variant(self, variant):
 obj = {'case': variant.name, 'type': self._use_type(variant.type)}
 return Annotated(obj, variant.ifcond)
@@ -268,9 +264,8 @@ def visit_object_type_flat(self, name, info, ifcond, 
features,
members, variants):
 obj = {'members': [self._gen_member(m) for m in members]}
 if variants:
-obj.update(self._gen_variants(variants.tag_member.name,
-  variants.variants))
-
+obj['tag'] = variants.tag_member.name
+obj['variants'] = [self._gen_variant(v) for v in variants.variants]
 self._gen_tree(name, 'object', obj, ifcond, features)
 
 def visit_alternate_type(self, name, info, ifcond, features, variants):
-- 
2.29.2

[PATCH v6 14/19] qapi/introspect.py: add type hint annotations

[John Snow]

NB: The type aliases (SchemaInfo et al) declare intent for some of the
"dictly-typed" objects we pass around in introspect.py. They do not
enforce the shape of those objects, and cannot, until Python 3.7 or
later. (And even then, it may not be "worth it".)

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 124 +++--
 scripts/qapi/mypy.ini  |   5 --
 scripts/qapi/schema.py |   2 +-
 3 files changed, 92 insertions(+), 39 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index b0fcc4443c1..45284af1330 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -17,6 +17,7 @@
 Iterable,
 List,
 Optional,
+Sequence,
 Tuple,
 TypeVar,
 Union,
@@ -30,10 +31,19 @@
 )
 from .gen import QAPISchemaMonolithicCVisitor
 from .schema import (
+QAPISchema,
 QAPISchemaArrayType,
 QAPISchemaBuiltinType,
+QAPISchemaEntity,
+QAPISchemaEnumMember,
+QAPISchemaFeature,
+QAPISchemaObjectType,
+QAPISchemaObjectTypeMember,
 QAPISchemaType,
+QAPISchemaVariant,
+QAPISchemaVariants,
 )
+from .source import QAPISourceInfo
 
 
 # This module constructs a tree data structure that is used to
@@ -58,6 +68,15 @@
 _Value = Union[_Scalar, _NonScalar]
 JSONValue = Union[_Value, 'Annotated[_Value]']
 
+# These types are based on structures defined in QEMU's schema, so we lack
+# precise types for them here. Python 3.6 does not offer TypedDict constructs,
+# so they are broadly typed here as simple Python Dicts.
+SchemaInfo = Dict[str, object]
+SchemaInfoObject = Dict[str, object]
+SchemaInfoObjectVariant = Dict[str, object]
+SchemaInfoObjectMember = Dict[str, object]
+SchemaInfoCommand = Dict[str, object]
+
 
 _ValueT = TypeVar('_ValueT', bound=_Value)
 
@@ -77,9 +96,11 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
 self.ifcond: Tuple[str, ...] = tuple(ifcond)
 
 
-def _tree_to_qlit(obj, level=0, dict_value=False):
+def _tree_to_qlit(obj: JSONValue,
+  level: int = 0,
+  dict_value: bool = False) -> str:
 
-def indent(level):
+def indent(level: int) -> str:
 return level * 4 * ' '
 
 if isinstance(obj, Annotated):
@@ -136,21 +157,21 @@ def indent(level):
 return ret
 
 
-def to_c_string(string):
+def to_c_string(string: str) -> str:
 return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
 
 
 class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
 
-def __init__(self, prefix, unmask):
+def __init__(self, prefix: str, unmask: bool):
 super().__init__(
 prefix, 'qapi-introspect',
 ' * QAPI/QMP schema introspection', __doc__)
 self._unmask = unmask
-self._schema = None
-self._trees = []
-self._used_types = []
-self._name_map = {}
+self._schema: Optional[QAPISchema] = None
+self._trees: List[Annotated[SchemaInfo]] = []
+self._used_types: List[QAPISchemaType] = []
+self._name_map: Dict[str, str] = {}
 self._genc.add(mcgen('''
 #include "qemu/osdep.h"
 #include "%(prefix)sqapi-introspect.h"
@@ -158,10 +179,10 @@ def __init__(self, prefix, unmask):
 ''',
  prefix=prefix))
 
-def visit_begin(self, schema):
+def visit_begin(self, schema: QAPISchema) -> None:
 self._schema = schema
 
-def visit_end(self):
+def visit_end(self) -> None:
 # visit the types that are actually used
 for typ in self._used_types:
 typ.visit(self)
@@ -183,18 +204,18 @@ def visit_end(self):
 self._used_types = []
 self._name_map = {}
 
-def visit_needed(self, entity):
+def visit_needed(self, entity: QAPISchemaEntity) -> bool:
 # Ignore types on first pass; visit_end() will pick up used types
 return not isinstance(entity, QAPISchemaType)
 
-def _name(self, name):
+def _name(self, name: str) -> str:
 if self._unmask:
 return name
 if name not in self._name_map:
 self._name_map[name] = '%d' % len(self._name_map)
 return self._name_map[name]
 
-def _use_type(self, typ):
+def _use_type(self, typ: QAPISchemaType) -> str:
 assert self._schema is not None
 
 # Map the various integer types to plain int
@@ -216,10 +237,13 @@ def _use_type(self, typ):
 return self._name(typ.name)
 
 @staticmethod
-def _gen_features(features):
+def _gen_features(features: List[QAPISchemaFeature]
+  ) -> List[Annotated[str]]:
 return [Annotated(f.name, f.ifcond) for f in features]
 
-def _gen_tree(self, name, mtype, obj, ifcond, features):
+def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
+  ifcond: Sequence[str],
+  features: Optional[List[QAPISchemaFeature]]) -> None:
 comment: Optional[str] = None

[PATCH v6 11/19] qapi/introspect.py: improve _tree_to_qlit error message

[John Snow]

Trivial; make the error message just a pinch more explicit in case we
trip this by accident in the future.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 5224be1a333..2ba0bfec733 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -125,7 +125,9 @@ def indent(level):
 elif isinstance(obj, bool):
 ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
 else:
-assert False# not implemented
+raise NotImplementedError(
+f"type '{type(obj).__name__}' not implemented"
+)
 if level > 0:
 ret += ','
 return ret
-- 
2.29.2

[PATCH v6 03/19] qapi/introspect.py: use _make_tree for features nodes

[John Snow]

At present, we open-code this in _make_tree itself; but if the structure
of the tree changes, this is brittle. Use an explicit recursive call to
_make_tree when appropriate to help keep the interior node typing
consistent.

A consequence of doing this is that the 'ifcond' key of the features
dict will be omitted when ifcond is false-ish, just like it is omitted
in top-level calls to _make_tree. This also increases consistency in our
handling of this property.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 43ab4be1f77..3295a15c98e 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -30,7 +30,9 @@ def _make_tree(obj, ifcond, features, extra=None):
 if ifcond:
 extra['if'] = ifcond
 if features:
-obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
+obj['features'] = [
+_make_tree(f.name, f.ifcond, None) for f in features
+]
 if extra:
 return (obj, extra)
 return obj
-- 
2.29.2

[PATCH v6 18/19] qapi/introspect.py: set _gen_tree's default ifcond argument to ()

[John Snow]

We don't need to create an empty, mutable list to pass to _gen_tree;
since it is now typed as a Sequence, we can use the empty tuple as a
default and omit the argument.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index e4d31a59503..c6f5cf8d874 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -252,7 +252,7 @@ def _gen_features(features: Sequence[QAPISchemaFeature]
 return [Annotated(f.name, f.ifcond) for f in features]
 
 def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
-  ifcond: Sequence[str],
+  ifcond: Sequence[str] = (),
   features: Sequence[QAPISchemaFeature] = ()) -> None:
 """
 Build and append a SchemaInfo object to self._trees.
@@ -299,7 +299,7 @@ def _gen_variant(self, variant: QAPISchemaVariant
 
 def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
json_type: str) -> None:
-self._gen_tree(name, 'builtin', {'json-type': json_type}, [])
+self._gen_tree(name, 'builtin', {'json-type': json_type})
 
 def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
 ifcond: Sequence[str],
-- 
2.29.2

[PATCH v6 09/19] qapi/introspect.py: Introduce preliminary tree typing

[John Snow]

The types will be used in forthcoming patches to add typing. These types
describe the layout and structure of the objects passed to
_tree_to_qlit, but lack the power to describe annotations until the next
commit.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 31 ++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 45231d2abc3..3c37c138013 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,7 +10,13 @@
 See the COPYING file in the top-level directory.
 """
 
-from typing import Optional
+from typing import (
+Any,
+Dict,
+List,
+Optional,
+Union,
+)
 
 from .common import (
 c_name,
@@ -26,6 +32,29 @@
 )
 
 
+# This module constructs a tree data structure that is used to
+# generate the introspection information for QEMU. It is shaped
+# like a JSON value.
+#
+# A complexity over JSON is that our values may or may not be annotated.
+#
+# Un-annotated values may be:
+# Scalar: str, bool, None.
+# Non-scalar: List, Dict
+# _value = Union[str, bool, None, Dict[str, JSONValue], List[JSONValue]]
+#
+# With optional annotations, the type of all values is:
+# JSONValue = Union[_Value, Annotated[_Value]]
+#
+# Sadly, mypy does not support recursive types; so the _Stub alias is used to
+# mark the imprecision in the type model where we'd otherwise use JSONValue.
+_Stub = Any
+_Scalar = Union[str, bool, None]
+_NonScalar = Union[Dict[str, _Stub], List[_Stub]]
+_Value = Union[_Scalar, _NonScalar]
+# JSONValue = TODO, in a forthcoming commit.
+
+
 def _make_tree(obj, ifcond, comment=None):
 extra = {
 'if': ifcond,
-- 
2.29.2

Re: [PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[Cleber Rosa]

On Mon, Feb 15, 2021 at 05:04:24PM -0500, John Snow wrote:
> On 2/11/21 5:01 PM, Cleber Rosa wrote:
> > Closing a file that is open for writing, and then reading from it
> > sounds like a better idea than the opposite, given that the content
> > will be flushed.
> > 
> > Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
> > Signed-off-by: Cleber Rosa 
> > ---
> >   python/qemu/machine.py | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > index 7a40f4604b..6e44bda337 100644
> > --- a/python/qemu/machine.py
> > +++ b/python/qemu/machine.py
> > @@ -337,12 +337,12 @@ class QEMUMachine:
> 
> Is there a way to improve context for python functions? What method is this
> in? etc.
> 
> >   self._qmp.close()
> >   self._qmp_connection = None
> > -self._load_io_log()
> > -
> >   if self._qemu_log_file is not None:
> >   self._qemu_log_file.close()
> >   self._qemu_log_file = None
> > +self._load_io_log()
> > +
> >   self._qemu_log_path = None
> >   if self._temp_dir is not None:
> > 
> 
> Yeh, seems fine, though as wainer points out the interdependencies between
> _load_io_log, _qemu_log_file and _qemu_log_path are not all strictly clear,
> so it seems fragile.
>

Yep, agreed.  This was a first, conservative change.  Expect more later.

> But, this is more correct than it was, so:
> 
> Reviewed-by: John Snow 

Thanks,
- Cleber


signature.asc
Description: PGP signature

[PATCH v6 02/19] qapi/introspect.py: assert schema is not None

[John Snow]

The introspect visitor is stateful, but expects that it will have a
schema to refer to. Add assertions that state this.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 5 +
 1 file changed, 5 insertions(+)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index fafec94e022..43ab4be1f77 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -147,6 +147,8 @@ def _name(self, name):
 return self._name_map[name]
 
 def _use_type(self, typ):
+assert self._schema is not None
+
 # Map the various integer types to plain int
 if typ.json_type() == 'int':
 typ = self._schema.lookup_type('int')
@@ -225,6 +227,8 @@ def visit_alternate_type(self, name, info, ifcond, 
features, variants):
 def visit_command(self, name, info, ifcond, features,
   arg_type, ret_type, gen, success_response, boxed,
   allow_oob, allow_preconfig, coroutine):
+assert self._schema is not None
+
 arg_type = arg_type or self._schema.the_empty_object_type
 ret_type = ret_type or self._schema.the_empty_object_type
 obj = {'arg-type': self._use_type(arg_type),
@@ -234,6 +238,7 @@ def visit_command(self, name, info, ifcond, features,
 self._gen_tree(name, 'command', obj, ifcond, features)
 
 def visit_event(self, name, info, ifcond, features, arg_type, boxed):
+assert self._schema is not None
 arg_type = arg_type or self._schema.the_empty_object_type
 self._gen_tree(name, 'event', {'arg-type': self._use_type(arg_type)},
ifcond, features)
-- 
2.29.2

[PATCH v6 07/19] qapi/introspect.py: replace 'extra' dict with 'comment' argument

[John Snow]

This is only used to pass in a dictionary with a comment already set, so
skip the runaround and just accept the (optional) comment.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 7730d8ed6b2..1655a21f85b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,6 +10,8 @@
 See the COPYING file in the top-level directory.
 """
 
+from typing import Optional
+
 from .common import (
 c_name,
 gen_endif,
@@ -24,11 +26,12 @@
 )
 
 
-def _make_tree(obj, ifcond, extra=None):
-if extra is None:
-extra = {}
+def _make_tree(obj, ifcond, comment=None):
+extra = {}
 if ifcond:
 extra['if'] = ifcond
+if comment:
+extra['comment'] = comment
 return (obj, extra)
 
 
@@ -174,18 +177,18 @@ def _gen_features(features):
 return [_make_tree(f.name, f.ifcond) for f in features]
 
 def _gen_tree(self, name, mtype, obj, ifcond, features):
-extra = None
+comment: Optional[str] = None
 if mtype not in ('command', 'event', 'builtin', 'array'):
 if not self._unmask:
 # Output a comment to make it easy to map masked names
 # back to the source when reading the generated output.
-extra = {'comment': '"%s" = %s' % (self._name(name), name)}
+comment = f'"{self._name(name)}" = {name}'
 name = self._name(name)
 obj['name'] = name
 obj['meta-type'] = mtype
 if features:
 obj['features'] = self._gen_features(features)
-self._trees.append(_make_tree(obj, ifcond, extra))
+self._trees.append(_make_tree(obj, ifcond, comment))
 
 def _gen_member(self, member):
 obj = {'name': member.name, 'type': self._use_type(member.type)}
-- 
2.29.2

[PATCH v6 08/19] qapi/introspect.py: Always define all 'extra' dict keys

[John Snow]

This mimics how a typed object works, where 'if' and 'comment' are
always set, regardless of if they have a value set or not.

It is safe to do this because of the way that _tree_to_qlit processes
these values (using dict.get with a default of None), resulting in no
change of output from _tree_to_qlit. There are no other users of this
data.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 1655a21f85b..45231d2abc3 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -27,11 +27,10 @@
 
 
 def _make_tree(obj, ifcond, comment=None):
-extra = {}
-if ifcond:
-extra['if'] = ifcond
-if comment:
-extra['comment'] = comment
+extra = {
+'if': ifcond,
+'comment': comment
+}
 return (obj, extra)
 
 
-- 
2.29.2

[PATCH v6 17/19] qapi/introspect.py: Type _gen_tree variants as Sequence[str]

[John Snow]

Optional[List] is clunky; an empty sequence can more elegantly convey
"no variants". By downgrading "List" to "Sequence", we can also accept
tuples; this is useful for the empty tuple specifically, which we may
use as a default parameter because it is immutable.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 649225988d1..e4d31a59503 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -247,13 +247,13 @@ def _use_type(self, typ: QAPISchemaType) -> str:
 return self._name(typ.name)
 
 @staticmethod
-def _gen_features(features: List[QAPISchemaFeature]
+def _gen_features(features: Sequence[QAPISchemaFeature]
   ) -> List[Annotated[str]]:
 return [Annotated(f.name, f.ifcond) for f in features]
 
 def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
   ifcond: Sequence[str],
-  features: Optional[List[QAPISchemaFeature]]) -> None:
+  features: Sequence[QAPISchemaFeature] = ()) -> None:
 """
 Build and append a SchemaInfo object to self._trees.
 
@@ -261,7 +261,8 @@ def _gen_tree(self, name: str, mtype: str, obj: Dict[str, 
object],
 :param mtype: The entity's meta-type.
 :param obj: Additional entity fields, as appropriate for the meta-type.
 :param ifcond: Sequence of conditionals that apply to this entity.
-:param features: Optional features field for SchemaInfo.
+:param features: Optional, The features field for SchemaInfo.
+ Will be omitted from the output if empty.
 """
 comment: Optional[str] = None
 if mtype not in ('command', 'event', 'builtin', 'array'):
@@ -298,7 +299,7 @@ def _gen_variant(self, variant: QAPISchemaVariant
 
 def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
json_type: str) -> None:
-self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
+self._gen_tree(name, 'builtin', {'json-type': json_type}, [])
 
 def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
 ifcond: Sequence[str],
@@ -316,7 +317,7 @@ def visit_array_type(self, name: str, info: 
Optional[QAPISourceInfo],
  element_type: QAPISchemaType) -> None:
 element = self._use_type(element_type)
 self._gen_tree('[' + element + ']', 'array', {'element-type': element},
-   ifcond, None)
+   ifcond)
 
 def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
ifcond: Sequence[str],
-- 
2.29.2

[RFC PATCH 21/23] i386/tdx: Use KVM_TDX_INIT_VCPU to pass HOB to TDVF

[Isaku Yamahata]

Specify the initial value for RCX/R8 to be the address of the HOB.
Don't propagate the value to Qemu's cache of the registers so as to
avoid implying that the register state is valid, e.g. Qemu doesn't model
TDX-SEAM behavior for initializing other GPRs.

Signed-off-by: Isaku Yamahata 
---
 target/i386/kvm/tdx.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index 49b4355849..007d33989b 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -268,10 +268,17 @@ out:
 
 void tdx_post_init_vcpu(CPUState *cpu)
 {
-CPUX86State *env = _CPU(cpu)->env;
+MachineState *ms = MACHINE(qdev_get_machine());
+TdxGuest *tdx = (TdxGuest *)object_dynamic_cast(OBJECT(ms->cgs),
+TYPE_TDX_GUEST);
+TdxFirmwareEntry *hob;
+
+if (!tdx) {
+return;
+}
 
-_tdx_ioctl(cpu, KVM_TDX_INIT_VCPU, 0,
-   (void *)(unsigned long)env->regs[R_ECX]);
+hob = tdx_get_hob_entry(tdx);
+_tdx_ioctl(cpu, KVM_TDX_INIT_VCPU, 0, (void *)hob->address);
 }
 
 static bool tdx_guest_get_debug(Object *obj, Error **errp)
-- 
2.17.1

[PATCH v6 15/19] qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit

[John Snow]

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 45284af1330..5d4f5e23f7e 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -99,6 +99,15 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
 def _tree_to_qlit(obj: JSONValue,
   level: int = 0,
   dict_value: bool = False) -> str:
+"""
+Convert the type tree into a QLIT C string, recursively.
+
+:param obj: The value to convert.
+This value may not be Annotated when dict_value is True.
+:param level: The indentation level for this particular value.
+:param dict_value: True when the value being processed belongs to a
+   dict key; which suppresses the output indent.
+"""
 
 def indent(level: int) -> str:
 return level * 4 * ' '
@@ -244,6 +253,15 @@ def _gen_features(features: List[QAPISchemaFeature]
 def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
   ifcond: Sequence[str],
   features: Optional[List[QAPISchemaFeature]]) -> None:
+"""
+Build and append a SchemaInfo object to self._trees.
+
+:param name: The entity's name.
+:param mtype: The entity's meta-type.
+:param obj: Additional entity fields, as appropriate for the meta-type.
+:param ifcond: Sequence of conditionals that apply to this entity.
+:param features: Optional features field for SchemaInfo.
+"""
 comment: Optional[str] = None
 if mtype not in ('command', 'event', 'builtin', 'array'):
 if not self._unmask:
-- 
2.29.2

[PATCH v6 05/19] qapi/introspect.py: guard against ifcond/comment misuse

[John Snow]

_tree_to_qlit is called recursively on dict values (isolated from their
keys); at such a point in generating output it is too late to apply an
ifcond. Similarly, comments do not necessarily have a "tidy" place they
can be printed in such a circumstance.

Forbid this usage by renaming "suppress_first_indent" to "dict_value" to
emphasize that indents are suppressed only for the benefit of dict
values; then add an assertion assuring we do not pass ifcond/comments
in this case.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 4749f65ea3c..a7ccda5ab92 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -34,7 +34,7 @@ def _make_tree(obj, ifcond, extra=None):
 return obj
 
 
-def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
+def _tree_to_qlit(obj, level=0, dict_value=False):
 
 def indent(level):
 return level * 4 * ' '
@@ -43,6 +43,12 @@ def indent(level):
 ifobj, extra = obj
 ifcond = extra.get('if')
 comment = extra.get('comment')
+
+# NB: _tree_to_qlit is called recursively on the values of a key:value
+# pair; those values can't be decorated with comments or conditionals.
+msg = "dict values cannot have attached comments or if-conditionals."
+assert not dict_value, msg
+
 ret = ''
 if comment:
 ret += indent(level) + '/* %s */\n' % comment
@@ -54,7 +60,7 @@ def indent(level):
 return ret
 
 ret = ''
-if not suppress_first_indent:
+if not dict_value:
 ret += indent(level)
 if obj is None:
 ret += 'QLIT_QNULL'
-- 
2.29.2

[RFC PATCH 20/23] i386/tdx: Add TDVF memory via INIT_MEM_REGION

[Isaku Yamahata]

Add, and optionally measure, TDVF memory via KVM_TDX_INIT_MEM_REGION as
part of finalizing the TD.

Signed-off-by: Isaku Yamahata 
Co-developed-by: Sean Christopherson 
Signed-off-by: Sean Christopherson 
---
 target/i386/kvm/tdx.c | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index 8e4bf98735..49b4355849 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -83,10 +83,26 @@ static void tdx_finalize_vm(Notifier *notifier, void 
*unused)
 {
 MachineState *ms = MACHINE(qdev_get_machine());
 TdxGuest *tdx = TDX_GUEST(ms->cgs);
+TdxFirmwareEntry *entry;
 
 tdvf_hob_create(tdx, tdx_get_hob_entry(tdx));
 
+for_each_fw_entry(>fw, entry) {
+struct kvm_tdx_init_mem_region mem_region = {
+.source_addr = (__u64)entry->mem_ptr,
+.gpa = entry->address,
+.nr_pages = entry->size / 4096,
+};
+
+__u32 metadata = entry->attributes & TDVF_SECTION_ATTRIBUTES_EXTENDMR ?
+ KVM_TDX_MEASURE_MEMORY_REGION : 0;
+
+tdx_ioctl(KVM_TDX_INIT_MEM_REGION, metadata, _region);
+}
+
 tdx_ioctl(KVM_TDX_FINALIZE_VM, 0, NULL);
+
+tdx->parent_obj.ready = true;
 }
 
 static Notifier tdx_machine_done_late_notify = {
@@ -288,9 +304,6 @@ static void tdx_guest_init(Object *obj)
 tdx->debug = false;
 object_property_add_bool(obj, "debug", tdx_guest_get_debug,
  tdx_guest_set_debug);
-
-/* TODO: move this after fully TD initialized */
-tdx->parent_obj.ready = true;
 }
 
 static void tdx_guest_finalize(Object *obj)
-- 
2.17.1

[PATCH v6 12/19] qapi/introspect.py: improve readability of _tree_to_qlit

[John Snow]

Subjective, but I find getting rid of the comprehensions helps. Also,
divide the sections into scalar and non-scalar sections, and remove
old-style string formatting.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 33 ++---
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 2ba0bfec733..afad891bb2b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -90,7 +90,7 @@ def indent(level):
 
 ret = ''
 if obj.comment:
-ret += indent(level) + '/* %s */\n' % obj.comment
+ret += indent(level) + f"/* {obj.comment} */\n"
 if obj.ifcond:
 ret += gen_if(obj.ifcond)
 ret += _tree_to_qlit(obj.value, level)
@@ -101,33 +101,36 @@ def indent(level):
 ret = ''
 if not dict_value:
 ret += indent(level)
+
+# Scalars:
 if obj is None:
 ret += 'QLIT_QNULL'
 elif isinstance(obj, str):
-ret += 'QLIT_QSTR(' + to_c_string(obj) + ')'
+ret += f"QLIT_QSTR({to_c_string(obj)})"
+elif isinstance(obj, bool):
+ret += f"QLIT_QBOOL({str(obj).lower()})"
+
+# Non-scalars:
 elif isinstance(obj, list):
-elts = [_tree_to_qlit(elt, level + 1).strip('\n')
-for elt in obj]
-elts.append(indent(level + 1) + "{}")
 ret += 'QLIT_QLIST(((QLitObject[]) {\n'
-ret += '\n'.join(elts) + '\n'
+for value in obj:
+ret += _tree_to_qlit(value, level + 1).strip('\n') + '\n'
+ret += indent(level + 1) + '{}\n'
 ret += indent(level) + '}))'
 elif isinstance(obj, dict):
-elts = []
-for key, value in sorted(obj.items()):
-elts.append(indent(level + 1) + '{ %s, %s }' %
-(to_c_string(key),
- _tree_to_qlit(value, level + 1, True)))
-elts.append(indent(level + 1) + '{}')
 ret += 'QLIT_QDICT(((QLitDictEntry[]) {\n'
-ret += ',\n'.join(elts) + '\n'
+for key, value in sorted(obj.items()):
+ret += indent(level + 1) + "{{ {:s}, {:s} }},\n".format(
+to_c_string(key),
+_tree_to_qlit(value, level + 1, dict_value=True)
+)
+ret += indent(level + 1) + '{}\n'
 ret += indent(level) + '}))'
-elif isinstance(obj, bool):
-ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
 else:
 raise NotImplementedError(
 f"type '{type(obj).__name__}' not implemented"
 )
+
 if level > 0:
 ret += ','
 return ret
-- 
2.29.2

[RFC PATCH 16/23] hw/i386: Add definitions from UEFI spec for volumes, resources, etc...

[Isaku Yamahata]

Add definitions for literals, enums, structs, GUIDs, etc... that will be
used by TDX to build the UEFI Hand-Off Block (HOB) that is passed to the
Trusted Domain Virtual Firmware (TDVF).  All values come from the UEFI
specification.

note: EFI_RESOURCE_ATTRIBUTE_{ENCRYPTED, UNACCEPTED}, will be added
in future UEFI spec.

Signed-off-by: Isaku Yamahata 
---
 hw/i386/uefi.h | 496 +
 1 file changed, 496 insertions(+)
 create mode 100644 hw/i386/uefi.h

diff --git a/hw/i386/uefi.h b/hw/i386/uefi.h
new file mode 100644
index 00..2ff6eeaa9e
--- /dev/null
+++ b/hw/i386/uefi.h
@@ -0,0 +1,496 @@
+/*
+ * Copyright (C) 2020 Intel Corporation
+ *
+ * Author: Isaku Yamahata 
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see .
+ *
+ */
+
+#ifndef HW_I386_UEFI_H
+#define HW_I386_UEFI_H
+
+/***/
+/*
+ * basic EFI definitions
+ * supplemented with UEFI Specification Version 2.8 (Errata A)
+ * released February 2020
+ */
+/* UEFI integer is little endian */
+
+typedef struct {
+uint32_t Data1;
+uint16_t Data2;
+uint16_t Data3;
+uint8_t Data4[8];
+} EFI_GUID;
+
+typedef uint64_t EFI_PHYSICAL_ADDRESS;
+typedef uint32_t EFI_BOOT_MODE;
+
+typedef enum {
+EfiReservedMemoryType,
+EfiLoaderCode,
+EfiLoaderData,
+EfiBootServicesCode,
+EfiBootServicesData,
+EfiRuntimeServicesCode,
+EfiRuntimeServicesData,
+EfiConventionalMemory,
+EfiUnusableMemory,
+EfiACPIReclaimMemory,
+EfiACPIMemoryNVS,
+EfiMemoryMappedIO,
+EfiMemoryMappedIOPortSpace,
+EfiPalCode,
+EfiPersistentMemory,
+EfiMaxMemoryType
+} EFI_MEMORY_TYPE;
+
+
+/*
+ * data structure firmware volume/file
+ * based on
+ * UEFI Platform Initialization Specification Version 1.7. vol 3, 3.2.1
+ */
+
+#define SIGNATURE_16(A, B)(((A) | (B << 8)))
+#define SIGNATURE_32(A, B, C, D)  (((A) | (B << 8) | (C << 16) | (D << 24)))
+#define SIGNATURE_64(A, B, C, D, E, F, G, H)\
+(SIGNATURE_32(A, B, C, D) | ((uint64_t) (SIGNATURE_32(E, F, G, H)) << 32))
+
+/***/
+/* Firmware Volume format */
+
+typedef uint32_t EFI_FV_FILE_ATTRIBUTES;
+
+
+#define EFI_FV_FILE_ATTRIB_ALIGNMENT 0x001F
+#define EFI_FV_FILE_ATTRIB_FIXED 0x0100
+#define EFI_FV_FILE_ATTRIB_MEMORY_MAPPED 0x0200
+
+typedef uint32_t EFI_FVB_ATTRIBUTES_2;
+
+
+#define EFI_FVB2_READ_DISABLED_CAP  0x0001
+#define EFI_FVB2_READ_ENABLED_CAP   0x0002
+#define EFI_FVB2_READ_STATUS0x0004
+#define EFI_FVB2_WRITE_DISABLED_CAP 0x0008
+#define EFI_FVB2_WRITE_ENABLED_CAP  0x0010
+#define EFI_FVB2_WRITE_STATUS   0x0020
+#define EFI_FVB2_LOCK_CAP   0x0040
+#define EFI_FVB2_LOCK_STATUS0x0080
+#define EFI_FVB2_STICKY_WRITE   0x0200
+#define EFI_FVB2_MEMORY_MAPPED  0x0400
+#define EFI_FVB2_ERASE_POLARITY 0x0800
+#define EFI_FVB2_READ_LOCK_CAP  0x1000
+#define EFI_FVB2_READ_LOCK_STATUS   0x2000
+#define EFI_FVB2_WRITE_LOCK_CAP 0x4000
+#define EFI_FVB2_WRITE_LOCK_STATUS  0x8000
+#define EFI_FVB2_ALIGNMENT  0x001F
+#define EFI_FVB2_WEAK_ALIGNMENT 0x8000
+#define EFI_FVB2_ALIGNMENT_10x
+#define EFI_FVB2_ALIGNMENT_20x0001
+#define EFI_FVB2_ALIGNMENT_40x0002
+#define EFI_FVB2_ALIGNMENT_80x0003
+#define EFI_FVB2_ALIGNMENT_16   0x0004
+#define EFI_FVB2_ALIGNMENT_32   0x0005
+#define EFI_FVB2_ALIGNMENT_64   0x0006
+#define EFI_FVB2_ALIGNMENT_128  0x0007
+#define EFI_FVB2_ALIGNMENT_256  0x0008
+#define EFI_FVB2_ALIGNMENT_512  0x0009
+#define EFI_FVB2_ALIGNMENT_1K   0x000A
+#define EFI_FVB2_ALIGNMENT_2K   0x000B
+#define EFI_FVB2_ALIGNMENT_4K   0x000C
+#define EFI_FVB2_ALIGNMENT_8K   0x000D
+#define EFI_FVB2_ALIGNMENT_16K  0x000E
+#define EFI_FVB2_ALIGNMENT_32K  0x000F
+#define EFI_FVB2_ALIGNMENT_64K  0x0010
+#define EFI_FVB2_ALIGNMENT_128K 0x0011
+#define EFI_FVB2_ALIGNMENT_256K 0x0012
+#define EFI_FVB2_ALIGNMENT_512K 0x0013
+#define EFI_FVB2_ALIGNMENT_1M   0x0014
+#define EFI_FVB2_ALIGNMENT_2M 

[PATCH v6 01/19] qapi: Replace List[str] with Sequence[str] for ifcond

[John Snow]

It does happen to be a list (as of now), but we can describe it in more
general terms with no loss in accuracy to allow tuples and other
constructs.

In the future, we can write "ifcond: Sequence[str] = ()" as a default
parameter, which we could not do with a Mutable type like a List.

Signed-off-by: John Snow 
---
 scripts/qapi/commands.py |  3 ++-
 scripts/qapi/events.py   |  4 ++--
 scripts/qapi/gen.py  | 12 ++--
 scripts/qapi/types.py| 12 ++--
 scripts/qapi/visit.py| 10 +-
 5 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 54af519f44d..0a75a9371ba 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -17,6 +17,7 @@
 Dict,
 List,
 Optional,
+Sequence,
 Set,
 )
 
@@ -297,7 +298,7 @@ def visit_end(self) -> None:
 def visit_command(self,
   name: str,
   info: Optional[QAPISourceInfo],
-  ifcond: List[str],
+  ifcond: Sequence[str],
   features: List[QAPISchemaFeature],
   arg_type: Optional[QAPISchemaObjectType],
   ret_type: Optional[QAPISchemaType],
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 8c57deb2b89..90d2f6156d8 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -12,7 +12,7 @@
 See the COPYING file in the top-level directory.
 """
 
-from typing import List, Optional
+from typing import List, Optional, Sequence
 
 from .common import c_enum_const, c_name, mcgen
 from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
@@ -214,7 +214,7 @@ def visit_end(self) -> None:
 def visit_event(self,
 name: str,
 info: Optional[QAPISourceInfo],
-ifcond: List[str],
+ifcond: Sequence[str],
 features: List[QAPISchemaFeature],
 arg_type: Optional[QAPISchemaObjectType],
 boxed: bool) -> None:
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 63549cc8d47..1fa503bdbdf 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -17,8 +17,8 @@
 from typing import (
 Dict,
 Iterator,
-List,
 Optional,
+Sequence,
 Tuple,
 )
 
@@ -85,7 +85,7 @@ def write(self, output_dir: str) -> None:
 fp.write(text)
 
 
-def _wrap_ifcond(ifcond: List[str], before: str, after: str) -> str:
+def _wrap_ifcond(ifcond: Sequence[str], before: str, after: str) -> str:
 if before == after:
 return after   # suppress empty #if ... #endif
 
@@ -127,9 +127,9 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
 class QAPIGenCCode(QAPIGen):
 def __init__(self, fname: str):
 super().__init__(fname)
-self._start_if: Optional[Tuple[List[str], str, str]] = None
+self._start_if: Optional[Tuple[Sequence[str], str, str]] = None
 
-def start_if(self, ifcond: List[str]) -> None:
+def start_if(self, ifcond: Sequence[str]) -> None:
 assert self._start_if is None
 self._start_if = (ifcond, self._body, self._preamble)
 
@@ -187,11 +187,11 @@ def _bottom(self) -> str:
 
 
 @contextmanager
-def ifcontext(ifcond: List[str], *args: QAPIGenCCode) -> Iterator[None]:
+def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) -> Iterator[None]:
 """
 A with-statement context manager that wraps with `start_if()` / `end_if()`.
 
-:param ifcond: A list of conditionals, passed to `start_if()`.
+:param ifcond: A sequence of conditionals, passed to `start_if()`.
 :param args: any number of `QAPIGenCCode`.
 
 Example::
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 2bdd6268476..20d572a23aa 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -13,7 +13,7 @@
 # See the COPYING file in the top-level directory.
 """
 
-from typing import List, Optional
+from typing import List, Optional, Sequence
 
 from .common import (
 c_enum_const,
@@ -139,7 +139,7 @@ def gen_struct_members(members: 
List[QAPISchemaObjectTypeMember]) -> str:
 return ret
 
 
-def gen_object(name: str, ifcond: List[str],
+def gen_object(name: str, ifcond: Sequence[str],
base: Optional[QAPISchemaObjectType],
members: List[QAPISchemaObjectTypeMember],
variants: Optional[QAPISchemaVariants]) -> str:
@@ -307,7 +307,7 @@ def _gen_type_cleanup(self, name: str) -> None:
 def visit_enum_type(self,
 name: str,
 info: Optional[QAPISourceInfo],
-ifcond: List[str],
+ifcond: Sequence[str],
 features: List[QAPISchemaFeature],
 members: List[QAPISchemaEnumMember],
 prefix: Optional[str]) -> None:
@@ -318,7 +318,7 @@ def visit_enum_type(self,
 

[PATCH v6 06/19] qapi/introspect.py: Unify return type of _make_tree()

[John Snow]

Returning two different types conditionally can be complicated to
type. Return one type for consistency.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index a7ccda5ab92..7730d8ed6b2 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -29,9 +29,7 @@ def _make_tree(obj, ifcond, extra=None):
 extra = {}
 if ifcond:
 extra['if'] = ifcond
-if extra:
-return (obj, extra)
-return obj
+return (obj, extra)
 
 
 def _tree_to_qlit(obj, level=0, dict_value=False):
-- 
2.29.2

[PATCH v6 04/19] qapi/introspect.py: add _gen_features helper

[John Snow]

_make_tree might receive a dict (a SchemaInfo object) or some other type
(usually, a string) for its obj parameter. Adding features information
should arguably be performed by the caller at such a time when we know
the type of the object and don't have to re-interrogate it.

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 3295a15c98e..4749f65ea3c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -24,15 +24,11 @@
 )
 
 
-def _make_tree(obj, ifcond, features, extra=None):
+def _make_tree(obj, ifcond, extra=None):
 if extra is None:
 extra = {}
 if ifcond:
 extra['if'] = ifcond
-if features:
-obj['features'] = [
-_make_tree(f.name, f.ifcond, None) for f in features
-]
 if extra:
 return (obj, extra)
 return obj
@@ -169,6 +165,10 @@ def _use_type(self, typ):
 return '[' + self._use_type(typ.element_type) + ']'
 return self._name(typ.name)
 
+@staticmethod
+def _gen_features(features):
+return [_make_tree(f.name, f.ifcond) for f in features]
+
 def _gen_tree(self, name, mtype, obj, ifcond, features):
 extra = None
 if mtype not in ('command', 'event', 'builtin', 'array'):
@@ -179,13 +179,17 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
 name = self._name(name)
 obj['name'] = name
 obj['meta-type'] = mtype
-self._trees.append(_make_tree(obj, ifcond, features, extra))
+if features:
+obj['features'] = self._gen_features(features)
+self._trees.append(_make_tree(obj, ifcond, extra))
 
 def _gen_member(self, member):
 obj = {'name': member.name, 'type': self._use_type(member.type)}
 if member.optional:
 obj['default'] = None
-return _make_tree(obj, member.ifcond, member.features)
+if member.features:
+obj['features'] = self._gen_features(member.features)
+return _make_tree(obj, member.ifcond)
 
 def _gen_variants(self, tag_name, variants):
 return {'tag': tag_name,
@@ -193,7 +197,7 @@ def _gen_variants(self, tag_name, variants):
 
 def _gen_variant(self, variant):
 obj = {'case': variant.name, 'type': self._use_type(variant.type)}
-return _make_tree(obj, variant.ifcond, None)
+return _make_tree(obj, variant.ifcond)
 
 def visit_builtin_type(self, name, info, json_type):
 self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
-- 
2.29.2

[PATCH v6 00/19] qapi: static typing conversion, pt2

[John Snow]

Hi, this series adds static type hints to the QAPI module.
This is part two, and covers introspect.py.

Part 2: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2
Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6

- Requires Python 3.6+
- Requires mypy 0.770 or newer (for type analysis only)
- Requires pylint 2.6.0 or newer (for lint checking only)

(Note: pylint does not like Python 3.9 very much yet. Known problem.)

Type hints are added in patches that add *only* type hints and change no
other behavior. Any necessary changes to behavior to accommodate typing
are split out into their own tiny patches.

Every commit should pass with (from ./scripts):
 - flake8 qapi/
 - pylint --rcfile=qapi/pylintrc qapi/
 - mypy --config-file=qapi/mypy.ini qapi/
 - isort -c qapi/

V6:

001/19:[down] 'qapi: Replace List[str] with Sequence[str] for ifcond'
009/19:[0021] [FC] 'qapi/introspect.py: Introduce preliminary tree typing'
010/19:[0013] [FC] 'qapi/introspect.py: create a typed 'Annotated' data 
strutcure'
013/19:[down] 'qapi/introspect.py: remove _gen_variants helper'
014/19:[0053] [FC] 'qapi/introspect.py: add type hint annotations'
015/19:[down] 'qapi/introspect.py: Add docstrings to _gen_tree and 
_tree_to_qlit'
017/19:[down] 'qapi/introspect.py: Type _gen_tree variants as Sequence[str]'
018/19:[down] 'qapi/introspect.py: set _gen_tree's default ifcond argument to 
()'
019/19:[down] 'qapi/introspect.py: add SchemaMetaType enum'

01: New; consistently type ifcond as Seq[str] in already-typed files.
09: Adjust comment concerning _stub to be more clear (?)
Rename _stub to _Stub, etc.
TreeValue becomes JSONValue.
10: _NodeT becomes _ValueT to match the _Value name.
Change visit_alternate_type whitespace around some more.
13: New, pre-requisite for using SchemaInfo aliases.
(Was not appropriate to go into #14.)
14: Use Sequence[str] instead of List[str] for ifcond
Use SchemaInfo "dummy types" instead of _DObject
15: Adjust comment to mention dict_value limitation.
Add docstring for _gen_tree (from former "dummy types" patch).
Change name of commit to reflect now-multiple docstring additions.

OPTIONAL PATCHES:

17: Use Sequence[QAPISchemaFeature] instead of Optional[List[QAPISchemaFeature]]
18: Set a default argument for ifcond to the empty tuple ().
Stylistically matches the above patch.
19: Create a SchemaMetaType enum and use it instead of the string type.
(Contains an optional blurb that can be removed if desired.)

V5:

04: Rename 'suppress_first_indent' to 'dict_value'
(Docstring added in 014.)
06: Avoid changing the output structure of _make_tree
07: Chance the structure of _make_tree 8-)
08: Change commented TreeValue to include a TODO instead.
09: Change NodeT bound to _value instead of TreeValue
Change "Remove in 3.7" text to include "TODO: "
Remove forwarding suppress_first_indent/dict_value in recursive cases
Change spacing in visit_alternate_type()
11: Consequence of suppress_first_value/dict_value change
12: Commit message note added
Changed _DObject comment
13: Commit notes adjusted
_DObject stuff: Comment near SchemaInfo et al adjusted
14: Changed docstring to reflect dict_value change
15: Updated copyright year for 2021 :~)

V4:
 - Rebased on "pt1.5" v4
 - signatures updated to use Optional[QAPISourceInfo]

John Snow (19):
  qapi: Replace List[str] with Sequence[str] for ifcond
  qapi/introspect.py: assert schema is not None
  qapi/introspect.py: use _make_tree for features nodes
  qapi/introspect.py: add _gen_features helper
  qapi/introspect.py: guard against ifcond/comment misuse
  qapi/introspect.py: Unify return type of _make_tree()
  qapi/introspect.py: replace 'extra' dict with 'comment' argument
  qapi/introspect.py: Always define all 'extra' dict keys
  qapi/introspect.py: Introduce preliminary tree typing
  qapi/introspect.py: create a typed 'Annotated' data strutcure
  qapi/introspect.py: improve _tree_to_qlit error message
  qapi/introspect.py: improve readability of _tree_to_qlit
  qapi/introspect.py: remove _gen_variants helper
  qapi/introspect.py: add type hint annotations
  qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit
  qapi/introspect.py: Update copyright and authors list
  qapi/introspect.py: Type _gen_tree variants as Sequence[str]
  qapi/introspect.py: set _gen_tree's default ifcond argument to ()
  qapi/introspect.py: add SchemaMetaType enum

 scripts/qapi/commands.py   |   3 +-
 scripts/qapi/events.py |   4 +-
 scripts/qapi/gen.py|  12 +-
 scripts/qapi/introspect.py | 350 +++--
 scripts/qapi/mypy.ini  |   5 -
 scripts/qapi/schema.py |   2 +-
 scripts/qapi/types.py  |  12 +-
 scripts/qapi/visit.py  |  10 +-
 8 files changed, 275 insertions(+), 123 deletions(-)

-- 
2.29.2

[RFC PATCH 15/23] i386/tdx: Add hook to require generic device loader

[Isaku Yamahata]

From: Sean Christopherson 

Add a hook for TDX to denote that the TD Virtual Firmware must be
provided via the "generic" device loader.  Error out if pflash is used
in conjuction with TDX.

Suggested-by: Isaku Yamahata 
Signed-off-by: Sean Christopherson 
---
 hw/i386/pc_sysfw.c |  6 ++
 include/sysemu/tdx.h   |  2 ++
 target/i386/kvm/tdx-stub.c |  5 +
 target/i386/kvm/tdx.c  | 25 +
 4 files changed, 38 insertions(+)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 11172214f1..65eed485ff 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -39,6 +39,7 @@
 #include "hw/block/flash.h"
 #include "sysemu/kvm.h"
 #include "sysemu/sev.h"
+#include "sysemu/tdx.h"
 
 #define FLASH_SECTOR_SIZE 4096
 
@@ -207,6 +208,11 @@ void pc_system_firmware_init(PCMachineState *pcms,
 int i;
 BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)];
 
+if (!tdx_system_firmware_init(pcms, rom_memory)) {
+pc_system_flash_cleanup_unused(pcms);
+return;
+}
+
 if (!pcmc->pci_enabled) {
 x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, true);
 return;
diff --git a/include/sysemu/tdx.h b/include/sysemu/tdx.h
index 03461b6ae8..70eb01348f 100644
--- a/include/sysemu/tdx.h
+++ b/include/sysemu/tdx.h
@@ -3,8 +3,10 @@
 
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/kvm.h"
+#include "hw/i386/pc.h"
 
 bool kvm_has_tdx(KVMState *s);
+int tdx_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 #endif
 
 void tdx_pre_create_vcpu(CPUState *cpu);
diff --git a/target/i386/kvm/tdx-stub.c b/target/i386/kvm/tdx-stub.c
index 93afe07ddb..4e1a0a4280 100644
--- a/target/i386/kvm/tdx-stub.c
+++ b/target/i386/kvm/tdx-stub.c
@@ -7,6 +7,11 @@ bool kvm_has_tdx(KVMState *s)
 {
 return false;
 }
+
+int tdx_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory)
+{
+return -ENOSYS;
+}
 #endif
 
 void tdx_pre_create_vcpu(CPUState *cpu)
diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index d095dab662..e8cd2a7672 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -106,6 +106,31 @@ int tdx_kvm_init(ConfidentialGuestSupport *cgs, Error 
**errp)
 return 0;
 }
 
+int tdx_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory)
+{
+MachineState *ms = MACHINE(pcms);
+TdxGuest *tdx = (TdxGuest *)object_dynamic_cast(OBJECT(ms->cgs),
+TYPE_TDX_GUEST);
+int i;
+
+if (!tdx) {
+return -ENOSYS;
+}
+
+/*
+ * Sanitiy check for tdx:
+ * TDX uses generic loader to load bios instead of pflash.
+ */
+for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
+if (drive_get(IF_PFLASH, 0, i)) {
+error_report("pflash not supported by VM type, "
+ "use -device loader,file=");
+exit(1);
+}
+}
+return 0;
+}
+
 void tdx_get_supported_cpuid(KVMState *s, uint32_t function,
  uint32_t index, int reg, uint32_t *ret)
 {
-- 
2.17.1

Re: [PATCH 1/3] target/arm: Add support for FEAT_SSBS, Speculative Store Bypass Safe

[Rebecca Cran]

On 2/15/21 5:19 PM, Richard Henderson wrote:

On 2/15/21 1:58 PM, Rebecca Cran wrote:

@@ -960,6 +960,12 @@ static void cpsr_write_from_spsr_elx(CPUARMState *env,
  val |= CPSR_DIT;
  }
  
+/* Move SSBS to the correct location for CPSR */

+if (val & PSTATE_SSBS) {
+val &= ~PSTATE_SSBS;
+val |= CPSR_SSBS;
+}


Incorrect.  SPSR_ELx leaves this at the same position as CPSR: bit 23.


Okay, I see where I'm going wrong now - SPSR_ELx has different formats 
for exceptions taken from aarch32 and aarch64 states, which I hadn't 
fully understood. I'll fix it.


--
Rebecca Cran

[RFC PATCH 23/23] target/i386: Add machine option to disable PIC/8259

[Isaku Yamahata]

From: Sean Christopherson 

Add a machine option to disable the legacy PIC (8259), which cannot be
supported for TDX guests as TDX-SEAM doesn't allow directly interrupt
injection.  Using posted interrupts for the PIC is not a viable option
as the guest BIOS/kernel will not do EOI for PIC IRQs, i.e. will leave
the vIRR bit set.

Signed-off-by: Sean Christopherson 
---
 hw/i386/pc.c | 18 ++
 hw/i386/pc_piix.c|  4 +++-
 hw/i386/pc_q35.c |  4 +++-
 include/hw/i386/pc.h |  2 ++
 4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8aa85dec54..12d44659bf 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1532,6 +1532,20 @@ static void pc_machine_set_hpet(Object *obj, bool value, 
Error **errp)
 pcms->hpet_enabled = value;
 }
 
+static bool pc_machine_get_pic(Object *obj, Error **errp)
+{
+PCMachineState *pcms = PC_MACHINE(obj);
+
+return pcms->pic_enabled;
+}
+
+static void pc_machine_set_pic(Object *obj, bool value, Error **errp)
+{
+PCMachineState *pcms = PC_MACHINE(obj);
+
+pcms->pic_enabled = value;
+}
+
 static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
 const char *name, void *opaque,
 Error **errp)
@@ -1672,6 +1686,7 @@ static void pc_machine_initfn(Object *obj)
 pcms->smbus_enabled = true;
 pcms->sata_enabled = true;
 pcms->pit_enabled = true;
+pcms->pic_enabled = true;
 pcms->max_fw_size = 8 * MiB;
 #ifdef CONFIG_HPET
 pcms->hpet_enabled = true;
@@ -1797,6 +1812,9 @@ static void pc_machine_class_init(ObjectClass *oc, void 
*data)
 object_class_property_add_bool(oc, PC_MACHINE_PIT,
 pc_machine_get_pit, pc_machine_set_pit);
 
+object_class_property_add_bool(oc, PC_MACHINE_PIC,
+pc_machine_get_pic, pc_machine_set_pic);
+
 object_class_property_add_bool(oc, "hpet",
 pc_machine_get_hpet, pc_machine_set_hpet);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 2904b40163..4b59d40c3c 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -220,7 +220,9 @@ static void pc_init1(MachineState *machine,
 }
 isa_bus_irqs(isa_bus, x86ms->gsi);
 
-pc_i8259_create(isa_bus, gsi_state->i8259_irq);
+if (pcms->pic_enabled) {
+pc_i8259_create(isa_bus, gsi_state->i8259_irq);
+}
 
 if (pcmc->pci_enabled) {
 ioapic_init_gsi(gsi_state, "i440fx");
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0a212443aa..c68799efbb 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -250,7 +250,9 @@ static void pc_q35_init(MachineState *machine)
 pci_bus_set_route_irq_fn(host_bus, ich9_route_intx_pin_to_irq);
 isa_bus = ich9_lpc->isa_bus;
 
-pc_i8259_create(isa_bus, gsi_state->i8259_irq);
+if (pcms->pic_enabled) {
+pc_i8259_create(isa_bus, gsi_state->i8259_irq);
+}
 
 if (pcmc->pci_enabled) {
 ioapic_init_gsi(gsi_state, "q35");
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 5f93540a43..6368f7bf77 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -44,6 +44,7 @@ typedef struct PCMachineState {
 bool sata_enabled;
 bool pit_enabled;
 bool hpet_enabled;
+bool pic_enabled;
 uint64_t max_fw_size;
 char *oem_id;
 char *oem_table_id;
@@ -63,6 +64,7 @@ typedef struct PCMachineState {
 #define PC_MACHINE_SMBUS"smbus"
 #define PC_MACHINE_SATA "sata"
 #define PC_MACHINE_PIT  "pit"
+#define PC_MACHINE_PIC  "pic"
 #define PC_MACHINE_MAX_FW_SIZE  "max-fw-size"
 #define PC_MACHINE_OEM_ID   "oem-id"
 #define PC_MACHINE_OEM_TABLE_ID "oem-table-id"
-- 
2.17.1

[RFC PATCH 18/23] i386/tdx: Parse tdvf metadata and store the result into TdxGuest

[Isaku Yamahata]

Add support for loading TDX's Trusted Domain Virtual Firmware (TDVF) via
the generic loader.  Prioritize the TDVF above plain hex to avoid false
positives with hex (TDVF has explicit metadata to confirm it's a TDVF).

Enumerate TempMem as added, private memory, i.e. E820_RESERVED,
otherwise TDVF will interpret the whole shebang as MMIO and complain
that the aperture overlaps other MMIO regions.

Signed-off-by: Isaku Yamahata 
Co-developed-by: Sean Christopherson 
Signed-off-by: Sean Christopherson 
---
 hw/core/generic-loader.c |   5 +
 hw/core/meson.build  |   3 +
 hw/core/tdvf-stub.c  |   6 +
 hw/i386/meson.build  |   1 +
 hw/i386/tdvf.c   | 305 +++
 include/sysemu/tdvf.h|   6 +
 target/i386/kvm/tdx.h|  26 
 7 files changed, 352 insertions(+)
 create mode 100644 hw/core/tdvf-stub.c
 create mode 100644 hw/i386/tdvf.c
 create mode 100644 include/sysemu/tdvf.h

diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
index 2b2a7b5e9a..c2f89bc0c9 100644
--- a/hw/core/generic-loader.c
+++ b/hw/core/generic-loader.c
@@ -35,6 +35,7 @@
 #include "hw/sysbus.h"
 #include "sysemu/dma.h"
 #include "sysemu/reset.h"
+#include "sysemu/tdvf.h"
 #include "hw/boards.h"
 #include "hw/loader.h"
 #include "hw/qdev-properties.h"
@@ -148,6 +149,10 @@ static void generic_loader_realize(DeviceState *dev, Error 
**errp)
   as);
 }
 
+if (size < 0) {
+size = load_tdvf(s->file);
+}
+
 if (size < 0) {
 size = load_targphys_hex_as(s->file, , as);
 }
diff --git a/hw/core/meson.build b/hw/core/meson.build
index 032576f571..d69a021c76 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -23,6 +23,9 @@ common_ss.add(when: 'CONFIG_REGISTER', if_true: 
files('register.c'))
 common_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
 common_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
 
+common_ss.add(when: 'CONFIG_TDX', if_false: files('tdvf-stub.c'))
+common_ss.add(when: 'CONFIG_ALL', if_true: files('tdvf-stub.c'))
+
 softmmu_ss.add(files(
   'fw-path-provider.c',
   'loader.c',
diff --git a/hw/core/tdvf-stub.c b/hw/core/tdvf-stub.c
new file mode 100644
index 00..5f2586dd70
--- /dev/null
+++ b/hw/core/tdvf-stub.c
@@ -0,0 +1,6 @@
+#include "sysemu/tdvf.h"
+
+int load_tdvf(const char *filename)
+{
+return -1;
+}
diff --git a/hw/i386/meson.build b/hw/i386/meson.build
index e5d109f5c6..945e805525 100644
--- a/hw/i386/meson.build
+++ b/hw/i386/meson.build
@@ -24,6 +24,7 @@ i386_ss.add(when: 'CONFIG_PC', if_true: files(
   'pc_sysfw.c',
   'acpi-build.c',
   'port92.c'))
+i386_ss.add(when: 'CONFIG_TDX', if_true: files('tdvf.c'))
 
 subdir('kvm')
 subdir('xen')
diff --git a/hw/i386/tdvf.c b/hw/i386/tdvf.c
new file mode 100644
index 00..bf0d5a9866
--- /dev/null
+++ b/hw/i386/tdvf.c
@@ -0,0 +1,305 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+
+ * Copyright (c) 2020 Intel Corporation
+ * Author: Isaku Yamahata 
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qemu/units.h"
+#include "cpu.h"
+#include "exec/hwaddr.h"
+#include "hw/boards.h"
+#include "hw/i386/e820_memory_layout.h"
+#include "hw/i386/tdvf.h"
+#include "hw/i386/x86.h"
+#include "hw/loader.h"
+#include "sysemu/tdx.h"
+#include "sysemu/tdvf.h"
+#include "target/i386/kvm/tdx.h"
+
+static void tdvf_init_ram_memory(MachineState *ms, TdxFirmwareEntry *entry)
+{
+void *ram_ptr = memory_region_get_ram_ptr(ms->ram);
+X86MachineState *x86ms = X86_MACHINE(ms);
+
+if (entry->type == TDVF_SECTION_TYPE_BFV ||
+entry->type == TDVF_SECTION_TYPE_CFV) {
+error_report("TDVF type %u addr 0x%" PRIx64 " in RAM (disallowed)",
+ entry->type, entry->address);
+exit(1);
+}
+
+if (entry->address < 4 * GiB) {
+entry->mem_ptr = ram_ptr + entry->address;
+} else {
+if (entry->address >= 4 * GiB + x86ms->above_4g_mem_size) {
+error_report("TDVF type %u address 0x%" PRIx64 " above high 
memory",
+ entry->type, entry->address);
+exit(1);
+}
+entry->mem_ptr = ram_ptr + 

[RFC PATCH 22/23] i386/tdx: Force x2apic mode and routing for TDs

[Isaku Yamahata]

From: Sean Christopherson 

TDX requires x2apic and "resets" vCPUs to have x2apic enabled.  Model
this in QEMU and unconditionally enable x2apic interrupt routing.

This fixes issues where interrupts from IRQFD would not get forwarded to
the guest due to KVM silently dropping the invalid routing entry.

Signed-off-by: Sean Christopherson 
---
 hw/intc/apic_common.c   | 12 
 include/hw/i386/apic.h  |  1 +
 include/hw/i386/apic_internal.h |  1 +
 target/i386/kvm/tdx.c   |  7 +++
 4 files changed, 21 insertions(+)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 97dd96dffa..6a69027377 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -263,6 +263,15 @@ void apic_designate_bsp(DeviceState *dev, bool bsp)
 }
 }
 
+void apic_force_x2apic(DeviceState *dev)
+{
+if (dev == NULL) {
+return;
+}
+
+APIC_COMMON(dev)->force_x2apic = true;
+}
+
 static void apic_reset_common(DeviceState *dev)
 {
 APICCommonState *s = APIC_COMMON(dev);
@@ -271,6 +280,9 @@ static void apic_reset_common(DeviceState *dev)
 
 bsp = s->apicbase & MSR_IA32_APICBASE_BSP;
 s->apicbase = APIC_DEFAULT_ADDRESS | bsp | MSR_IA32_APICBASE_ENABLE;
+if (s->force_x2apic) {
+s->apicbase |= MSR_IA32_APICBASE_EXTD;
+}
 s->id = s->initial_apic_id;
 
 apic_reset_irq_delivered();
diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
index da1d2fe155..7d05abd7e0 100644
--- a/include/hw/i386/apic.h
+++ b/include/hw/i386/apic.h
@@ -19,6 +19,7 @@ void apic_init_reset(DeviceState *s);
 void apic_sipi(DeviceState *s);
 void apic_poll_irq(DeviceState *d);
 void apic_designate_bsp(DeviceState *d, bool bsp);
+void apic_force_x2apic(DeviceState *d);
 int apic_get_highest_priority_irr(DeviceState *dev);
 
 /* pc.c */
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index c175e7e718..eda0b5a587 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -187,6 +187,7 @@ struct APICCommonState {
 DeviceState *vapic;
 hwaddr vapic_paddr; /* note: persistence via kvmvapic */
 bool legacy_instance_id;
+bool force_x2apic;
 };
 
 typedef struct VAPICState {
diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index 007d33989b..b4bd157fe1 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -137,6 +137,11 @@ int tdx_kvm_init(ConfidentialGuestSupport *cgs, Error 
**errp)
 tdx_caps->nr_cpuid_configs = TDX1_MAX_NR_CPUID_CONFIGS;
 tdx_ioctl(KVM_TDX_CAPABILITIES, 0, tdx_caps);
 
+if (!kvm_enable_x2apic()) {
+error_report("Failed to enable x2apic in KVM");
+exit(1);
+}
+
 qemu_add_machine_init_done_late_notifier(_machine_done_late_notify);
 return 0;
 }
@@ -279,6 +284,8 @@ void tdx_post_init_vcpu(CPUState *cpu)
 
 hob = tdx_get_hob_entry(tdx);
 _tdx_ioctl(cpu, KVM_TDX_INIT_VCPU, 0, (void *)hob->address);
+
+apic_force_x2apic(X86_CPU(cpu)->apic_state);
 }
 
 static bool tdx_guest_get_debug(Object *obj, Error **errp)
-- 
2.17.1

[RFC PATCH 13/23] i386/tdx: Frame in tdx_get_supported_cpuid with KVM_TDX_CAPABILITIES

[Isaku Yamahata]

From: Sean Christopherson 

Add support for grabbing KVM_TDX_CAPABILITIES and use the new
kvm_get_supported_cpuid() hook to adjust the supported XCR0 bits.

Add TODOs for the remaining work.

Signed-off-by: Sean Christopherson 
---
 target/i386/kvm/kvm.c |  2 ++
 target/i386/kvm/tdx.c | 84 ++-
 target/i386/kvm/tdx.h |  2 ++
 3 files changed, 80 insertions(+), 8 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 018a757dc6..e6f7015be8 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -441,6 +441,8 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t 
function,
 ret |= 1U << KVM_HINTS_REALTIME;
 }
 
+tdx_get_supported_cpuid(s, function, index, reg, );
+
 return ret;
 }
 
diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index d8b79e975f..9d4195a705 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -21,6 +21,7 @@
 #include "hw/boards.h"
 #include "qapi/error.h"
 #include "qom/object_interfaces.h"
+#include "standard-headers/asm-x86/kvm_para.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "sysemu/kvm_int.h"
@@ -47,7 +48,11 @@ static void __tdx_ioctl(int ioctl_no, const char *ioctl_name,
 tdx_cmd.metadata = metadata;
 tdx_cmd.data = (__u64)(unsigned long)data;
 
-r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, _cmd);
+if (ioctl_no == KVM_TDX_CAPABILITIES) {
+r = kvm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, _cmd);
+} else {
+r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, _cmd);
+}
 if (r) {
 error_report("%s failed: %s", ioctl_name, strerror(-r));
 exit(1);
@@ -65,17 +70,83 @@ static Notifier tdx_machine_done_late_notify = {
 .notify = tdx_finalize_vm,
 };
 
+#define TDX1_MAX_NR_CPUID_CONFIGS 6
+
+static struct {
+struct kvm_tdx_capabilities __caps;
+struct kvm_tdx_cpuid_config __cpuid_configs[TDX1_MAX_NR_CPUID_CONFIGS];
+} __tdx_caps;
+
+static struct kvm_tdx_capabilities *tdx_caps = (void *)&__tdx_caps;
+
+#define XCR0_MASK (MAKE_64BIT_MASK(0, 8) | BIT_ULL(9))
+#define XSS_MASK (~XCR0_MASK)
+
 int tdx_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
 {
 TdxGuest *tdx = (TdxGuest *)object_dynamic_cast(OBJECT(cgs),
 TYPE_TDX_GUEST);
-if (tdx) {
-qemu_add_machine_init_done_late_notifier(
-_machine_done_late_notify);
+if (!tdx) {
+return 0;
 }
+
+QEMU_BUILD_BUG_ON(sizeof(__tdx_caps) !=
+  sizeof(struct kvm_tdx_capabilities) +
+  sizeof(struct kvm_tdx_cpuid_config) *
+  TDX1_MAX_NR_CPUID_CONFIGS);
+
+tdx_caps->nr_cpuid_configs = TDX1_MAX_NR_CPUID_CONFIGS;
+tdx_ioctl(KVM_TDX_CAPABILITIES, 0, tdx_caps);
+
+qemu_add_machine_init_done_late_notifier(_machine_done_late_notify);
 return 0;
 }
 
+void tdx_get_supported_cpuid(KVMState *s, uint32_t function,
+ uint32_t index, int reg, uint32_t *ret)
+{
+MachineState *ms = MACHINE(qdev_get_machine());
+TdxGuest *tdx = (TdxGuest *)object_dynamic_cast(OBJECT(ms->cgs),
+TYPE_TDX_GUEST);
+
+if (!tdx) {
+return;
+}
+
+switch (function) {
+case 1:
+if (reg == R_ECX) {
+*ret &= ~CPUID_EXT_VMX;
+}
+break;
+case 0xd:
+if (index == 0) {
+if (reg == R_EAX) {
+*ret &= (uint32_t)tdx_caps->xfam_fixed0 & XCR0_MASK;
+*ret |= (uint32_t)tdx_caps->xfam_fixed1 & XCR0_MASK;
+} else if (reg == R_EDX) {
+*ret &= (tdx_caps->xfam_fixed0 & XCR0_MASK) >> 32;
+*ret |= (tdx_caps->xfam_fixed1 & XCR0_MASK) >> 32;
+}
+} else if (index == 1) {
+/* TODO: Adjust XSS when it's supported. */
+}
+break;
+case KVM_CPUID_FEATURES:
+if (reg == R_EAX) {
+*ret &= ~((1ULL << KVM_FEATURE_CLOCKSOURCE) |
+  (1ULL << KVM_FEATURE_CLOCKSOURCE2) |
+  (1ULL << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT) |
+  (1ULL << KVM_FEATURE_ASYNC_PF) |
+  (1ULL << KVM_FEATURE_ASYNC_PF_VMEXIT));
+}
+break;
+default:
+/* TODO: Use tdx_caps to adjust CPUID leafs. */
+break;
+}
+}
+
 void tdx_pre_create_vcpu(CPUState *cpu)
 {
 struct {
@@ -103,10 +174,7 @@ void tdx_pre_create_vcpu(CPUState *cpu)
 return;
 }
 
-/* HACK: Remove MPX support, which is not allowed by TDX. */
-env->features[FEAT_XSAVE_COMP_LO] &= ~(XSTATE_BNDREGS_MASK |
-   XSTATE_BNDCSR_MASK);
-
+/* TODO: Use tdx_caps to validate the config. */
 if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
 error_report("TDX VM must support XSAVE features");
 exit(1);

[RFC PATCH 14/23] i386/tdx: Frame in the call for KVM_TDX_INIT_VCPU

[Isaku Yamahata]

Signed-off-by: Sean Christopherson 
---
 include/sysemu/tdx.h   |  1 +
 target/i386/kvm/kvm.c  |  8 
 target/i386/kvm/tdx-stub.c |  4 
 target/i386/kvm/tdx.c  | 20 
 4 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/include/sysemu/tdx.h b/include/sysemu/tdx.h
index 36a901e723..03461b6ae8 100644
--- a/include/sysemu/tdx.h
+++ b/include/sysemu/tdx.h
@@ -8,5 +8,6 @@ bool kvm_has_tdx(KVMState *s);
 #endif
 
 void tdx_pre_create_vcpu(CPUState *cpu);
+void tdx_post_init_vcpu(CPUState *cpu);
 
 #endif
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index e6f7015be8..52dbccedb5 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -4034,6 +4034,14 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
 
 assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
 
+/*
+ * level == KVM_PUT_FULL_STATE is only set by
+ * kvm_cpu_synchronize_post_init() after initialization
+ */
+if (vm_type == KVM_X86_TDX_VM && level == KVM_PUT_FULL_STATE) {
+tdx_post_init_vcpu(cpu);
+}
+
 /* TODO: Allow accessing guest state for debug TDs. */
 if (vm_type == KVM_X86_TDX_VM) {
 return 0;
diff --git a/target/i386/kvm/tdx-stub.c b/target/i386/kvm/tdx-stub.c
index 93d5913c89..93afe07ddb 100644
--- a/target/i386/kvm/tdx-stub.c
+++ b/target/i386/kvm/tdx-stub.c
@@ -12,3 +12,7 @@ bool kvm_has_tdx(KVMState *s)
 void tdx_pre_create_vcpu(CPUState *cpu)
 {
 }
+
+void tdx_post_init_vcpu(CPUState *cpu)
+{
+}
diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index 9d4195a705..d095dab662 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -36,7 +36,7 @@ bool kvm_has_tdx(KVMState *s)
 return !!(kvm_check_extension(s, KVM_CAP_VM_TYPES) & BIT(KVM_X86_TDX_VM));
 }
 
-static void __tdx_ioctl(int ioctl_no, const char *ioctl_name,
+static void __tdx_ioctl(void *state, int ioctl_no, const char *ioctl_name,
 __u32 metadata, void *data)
 {
 struct kvm_tdx_cmd tdx_cmd;
@@ -49,17 +49,21 @@ static void __tdx_ioctl(int ioctl_no, const char 
*ioctl_name,
 tdx_cmd.data = (__u64)(unsigned long)data;
 
 if (ioctl_no == KVM_TDX_CAPABILITIES) {
-r = kvm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, _cmd);
+r = kvm_ioctl(state, KVM_MEMORY_ENCRYPT_OP, _cmd);
+} else if (ioctl_no == KVM_TDX_INIT_VCPU) {
+r = kvm_vcpu_ioctl(state, KVM_MEMORY_ENCRYPT_OP, _cmd);
 } else {
-r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, _cmd);
+r = kvm_vm_ioctl(state, KVM_MEMORY_ENCRYPT_OP, _cmd);
 }
 if (r) {
 error_report("%s failed: %s", ioctl_name, strerror(-r));
 exit(1);
 }
 }
+#define _tdx_ioctl(cpu, ioctl_no, metadata, data) \
+__tdx_ioctl(cpu, ioctl_no, stringify(ioctl_no), metadata, data)
 #define tdx_ioctl(ioctl_no, metadata, data) \
-__tdx_ioctl(ioctl_no, stringify(ioctl_no), metadata, data)
+_tdx_ioctl(kvm_state, ioctl_no, metadata, data)
 
 static void tdx_finalize_vm(Notifier *notifier, void *unused)
 {
@@ -202,6 +206,14 @@ out:
 qemu_mutex_unlock(>lock);
 }
 
+void tdx_post_init_vcpu(CPUState *cpu)
+{
+CPUX86State *env = _CPU(cpu)->env;
+
+_tdx_ioctl(cpu, KVM_TDX_INIT_VCPU, 0,
+   (void *)(unsigned long)env->regs[R_ECX]);
+}
+
 static bool tdx_guest_get_debug(Object *obj, Error **errp)
 {
 TdxGuest *tdx = TDX_GUEST(obj);
-- 
2.17.1

[RFC PATCH 11/23] hw/i386: Initialize TDX via KVM ioctl() when kvm_type is TDX

[Isaku Yamahata]

From: Xiaoyao Li 

Introduce tdx_ioctl() to invoke TDX specific sub-ioctls of
KVM_MEMORY_ENCRYPT_OP.  Use tdx_ioctl() to invoke KVM_TDX_INIT, by way
of tdx_init(), during kvm_arch_init().  KVM_TDX_INIT configures global
TD state, e.g. the canonical CPUID config, and must be executed prior to
creating vCPUs.

Note, this doesn't address the fact that Qemu may change the CPUID
configuration when creating vCPUs, i.e. punts on refactoring Qemu to
provide a stable CPUID config prior to kvm_arch_init().

Explicitly set subleaf index and flags when adding CPUID
Set the index and flags when adding a CPUID entry to avoid propagating
stale state from a removed entry, e.g. when the CPUID 0x4 loop bails, it
can leave non-zero index and flags in the array.

Signed-off-by: Xiaoyao Li 
Co-developed-by: Sean Christopherson 
Signed-off-by: Sean Christopherson 
---
 accel/kvm/kvm-all.c|   2 +
 include/sysemu/tdx.h   |   2 +
 target/i386/kvm/tdx-stub.c |   4 ++
 target/i386/kvm/tdx.c  | 128 +
 target/i386/kvm/tdx.h  |  24 +++
 5 files changed, 160 insertions(+)
 create mode 100644 target/i386/kvm/tdx.h

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 351c25a5cb..cd13b8c94d 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -40,6 +40,7 @@
 #include "trace.h"
 #include "hw/irq.h"
 #include "sysemu/sev.h"
+#include "sysemu/tdx.h"
 #include "qapi/visitor.h"
 #include "qapi/qapi-types-common.h"
 #include "qapi/qapi-visit-common.h"
@@ -428,6 +429,7 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
 
 trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
 
+tdx_pre_create_vcpu(cpu);
 ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
 if (ret < 0) {
 error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed 
(%lu)",
diff --git a/include/sysemu/tdx.h b/include/sysemu/tdx.h
index 60ebded851..36a901e723 100644
--- a/include/sysemu/tdx.h
+++ b/include/sysemu/tdx.h
@@ -7,4 +7,6 @@
 bool kvm_has_tdx(KVMState *s);
 #endif
 
+void tdx_pre_create_vcpu(CPUState *cpu);
+
 #endif
diff --git a/target/i386/kvm/tdx-stub.c b/target/i386/kvm/tdx-stub.c
index e1eb09cae1..93d5913c89 100644
--- a/target/i386/kvm/tdx-stub.c
+++ b/target/i386/kvm/tdx-stub.c
@@ -8,3 +8,7 @@ bool kvm_has_tdx(KVMState *s)
 return false;
 }
 #endif
+
+void tdx_pre_create_vcpu(CPUState *cpu)
+{
+}
diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index e62a570f75..00eda80725 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -14,8 +14,10 @@
 #include "qemu/osdep.h"
 
 #include 
+#include 
 
 #include "cpu.h"
+#include "kvm_i386.h"
 #include "hw/boards.h"
 #include "qapi/error.h"
 #include "qom/object_interfaces.h"
@@ -23,8 +25,134 @@
 #include "sysemu/kvm.h"
 #include "sysemu/kvm_int.h"
 #include "sysemu/tdx.h"
+#include "tdx.h"
+
+#define TDX1_TD_ATTRIBUTE_DEBUG BIT_ULL(0)
+#define TDX1_TD_ATTRIBUTE_PERFMON BIT_ULL(63)
 
 bool kvm_has_tdx(KVMState *s)
 {
 return !!(kvm_check_extension(s, KVM_CAP_VM_TYPES) & BIT(KVM_X86_TDX_VM));
 }
+
+static void __tdx_ioctl(int ioctl_no, const char *ioctl_name,
+__u32 metadata, void *data)
+{
+struct kvm_tdx_cmd tdx_cmd;
+int r;
+
+memset(_cmd, 0x0, sizeof(tdx_cmd));
+
+tdx_cmd.id = ioctl_no;
+tdx_cmd.metadata = metadata;
+tdx_cmd.data = (__u64)(unsigned long)data;
+
+r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, _cmd);
+if (r) {
+error_report("%s failed: %s", ioctl_name, strerror(-r));
+exit(1);
+}
+}
+#define tdx_ioctl(ioctl_no, metadata, data) \
+__tdx_ioctl(ioctl_no, stringify(ioctl_no), metadata, data)
+
+void tdx_pre_create_vcpu(CPUState *cpu)
+{
+struct {
+struct kvm_cpuid2 cpuid;
+struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
+} cpuid_data;
+
+/*
+ * The kernel defines these structs with padding fields so there
+ * should be no extra padding in our cpuid_data struct.
+ */
+QEMU_BUILD_BUG_ON(sizeof(cpuid_data) !=
+  sizeof(struct kvm_cpuid2) +
+  sizeof(struct kvm_cpuid_entry2) *
+  KVM_MAX_CPUID_ENTRIES);
+
+MachineState *ms = MACHINE(qdev_get_machine());
+X86CPU *x86cpu = X86_CPU(cpu);
+CPUX86State *env = >env;
+TdxGuest *tdx = (TdxGuest *)object_dynamic_cast(OBJECT(ms->cgs),
+TYPE_TDX_GUEST);
+struct kvm_tdx_init_vm init_vm;
+
+if (!tdx) {
+return;
+}
+
+/* HACK: Remove MPX support, which is not allowed by TDX. */
+env->features[FEAT_XSAVE_COMP_LO] &= ~(XSTATE_BNDREGS_MASK |
+   XSTATE_BNDCSR_MASK);
+
+if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
+error_report("TDX VM must support XSAVE features");
+exit(1);
+}
+
+qemu_mutex_lock(>lock);
+if (tdx->initialized) {
+goto out;
+}
+

[RFC PATCH 01/23] target/i386: Expose x86_cpu_get_supported_feature_word() for TDX

[Isaku Yamahata]

From: Sean Christopherson 

Expose x86_cpu_get_supported_feature_word() outside of cpu.c so that it
can be used by TDX to setup the VM-wide CPUID configuration.

Signed-off-by: Sean Christopherson 
---
 target/i386/cpu.c | 4 ++--
 target/i386/cpu.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 9c3d2d60b7..578e1fe25f 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5043,8 +5043,8 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error 
**errp)
 return cpu_list;
 }
 
-static uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
-   bool migratable_only)
+uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
+bool migratable_only)
 {
 FeatureWordInfo *wi = _word_info[w];
 uint64_t r = 0;
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 8d599bb5b8..7274e8d1b4 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1815,6 +1815,9 @@ void cpu_set_ignne(void);
 /* mpx_helper.c */
 void cpu_sync_bndcs_hflags(CPUX86State *env);
 
+uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
+bool migratable_only);
+
 /* this function must always be used to load data in the segment
cache: it synchronizes the hflags with the segment cache values */
 static inline void cpu_x86_load_seg_cache(CPUX86State *env,
-- 
2.17.1

[RFC PATCH 19/23] i386/tdx: Create the TD HOB list upon machine init done

[Isaku Yamahata]

Build the TD HOB during machine late initialization, i.e. once guest
memory is fully defined.
Note, the attribute absolutely for MMIO HOB entries must include
UNCACHEABLE, else TDVF will effectively consider it a bad HOB entry
and ignore it.

Signed-off-by: Isaku Yamahata 
Co-developed-by: Sean Christopherson 
Signed-off-by: Sean Christopherson 
---
 hw/i386/meson.build   |   2 +-
 hw/i386/tdvf-hob.c| 226 ++
 hw/i386/tdvf-hob.h|  25 +
 target/i386/kvm/tdx.c |  19 
 4 files changed, 271 insertions(+), 1 deletion(-)
 create mode 100644 hw/i386/tdvf-hob.c
 create mode 100644 hw/i386/tdvf-hob.h

diff --git a/hw/i386/meson.build b/hw/i386/meson.build
index 945e805525..8175c3c638 100644
--- a/hw/i386/meson.build
+++ b/hw/i386/meson.build
@@ -24,7 +24,7 @@ i386_ss.add(when: 'CONFIG_PC', if_true: files(
   'pc_sysfw.c',
   'acpi-build.c',
   'port92.c'))
-i386_ss.add(when: 'CONFIG_TDX', if_true: files('tdvf.c'))
+i386_ss.add(when: 'CONFIG_TDX', if_true: files('tdvf.c', 'tdvf-hob.c'))
 
 subdir('kvm')
 subdir('xen')
diff --git a/hw/i386/tdvf-hob.c b/hw/i386/tdvf-hob.c
new file mode 100644
index 00..c37fb22396
--- /dev/null
+++ b/hw/i386/tdvf-hob.c
@@ -0,0 +1,226 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+
+ * Copyright (c) 2020 Intel Corporation
+ * Author: Isaku Yamahata 
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "e820_memory_layout.h"
+#include "hw/i386/pc.h"
+#include "hw/i386/x86.h"
+#include "hw/pci/pci_host.h"
+#include "sysemu/tdx.h"
+#include "tdvf-hob.h"
+#include "uefi.h"
+
+typedef struct TdvfHob {
+hwaddr hob_addr;
+void *ptr;
+int size;
+
+/* working area */
+void *current;
+void *end;
+} TdvfHob;
+
+static uint64_t tdvf_current_guest_addr(const TdvfHob *hob)
+{
+return hob->hob_addr + (hob->current - hob->ptr);
+}
+
+static void tdvf_align(TdvfHob *hob, size_t align)
+{
+hob->current = QEMU_ALIGN_PTR_UP(hob->current, align);
+}
+
+static void *tdvf_get_area(TdvfHob *hob, uint64_t size)
+{
+void *ret;
+
+if (hob->current + size > hob->end) {
+error_report("TD_HOB overrun, size = 0x%" PRIx64, size);
+exit(1);
+}
+
+ret = hob->current;
+hob->current += size;
+tdvf_align(hob, 8);
+return ret;
+}
+
+static void tdvf_hob_add_mmio_resource(TdvfHob *hob, uint64_t start,
+   uint64_t end)
+{
+EFI_HOB_RESOURCE_DESCRIPTOR *region;
+
+if (!start) {
+return;
+}
+
+region = tdvf_get_area(hob, sizeof(*region));
+*region = (EFI_HOB_RESOURCE_DESCRIPTOR) {
+.Header = {
+.HobType = EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
+.HobLength = cpu_to_le16(sizeof(*region)),
+.Reserved = cpu_to_le32(0),
+},
+.Owner = EFI_HOB_OWNER_ZERO,
+.ResourceType = cpu_to_le32(EFI_RESOURCE_MEMORY_MAPPED_IO),
+.ResourceAttribute = cpu_to_le32(EFI_RESOURCE_ATTRIBUTE_TDVF_MMIO),
+.PhysicalStart = cpu_to_le64(start),
+.ResourceLength = cpu_to_le64(end - start),
+};
+}
+
+static void tdvf_hob_add_mmio_resources(TdvfHob *hob)
+{
+MachineState *ms = MACHINE(qdev_get_machine());
+X86MachineState *x86ms = X86_MACHINE(ms);
+PCIHostState *pci_host;
+uint64_t start, end;
+Object *host;
+
+/* Effectively PCI hole + other MMIO devices. */
+tdvf_hob_add_mmio_resource(hob, x86ms->below_4g_mem_size,
+   APIC_DEFAULT_ADDRESS);
+
+/* Stolen from acpi_get_i386_pci_host(), there's gotta be an easier way. */
+pci_host = OBJECT_CHECK(PCIHostState,
+object_resolve_path("/machine/i440fx", NULL),
+TYPE_PCI_HOST_BRIDGE);
+if (!pci_host) {
+pci_host = OBJECT_CHECK(PCIHostState,
+object_resolve_path("/machine/q35", NULL),
+TYPE_PCI_HOST_BRIDGE);
+}
+g_assert(pci_host);
+
+host = OBJECT(pci_host);
+
+/* PCI hole above 4gb. */
+start = object_property_get_uint(host, PCI_HOST_PROP_PCI_HOLE64_START,
+ NULL);
+end = object_property_get_uint(host, PCI_HOST_PROP_PCI_HOLE64_END, NULL);
+tdvf_hob_add_mmio_resource(hob, start, 

[RFC PATCH 10/23] linux-headers: Update headers to pull in TDX API changes

[Isaku Yamahata]

From: Xiaoyao Li 

Pull in recent TDX updates, which are not backwards compatible.

Signed-off-by: Xiaoyao Li 
Co-developed-by: Sean Christopherson 
Signed-off-by: Sean Christopherson 
---
 linux-headers/asm-x86/kvm.h | 55 +
 linux-headers/linux/kvm.h   |  2 ++
 2 files changed, 57 insertions(+)

diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h
index 8e76d3701d..26d8197e41 100644
--- a/linux-headers/asm-x86/kvm.h
+++ b/linux-headers/asm-x86/kvm.h
@@ -487,4 +487,59 @@ struct kvm_pmu_event_filter {
 #define KVM_PMU_EVENT_ALLOW 0
 #define KVM_PMU_EVENT_DENY 1
 
+#define KVM_X86_LEGACY_VM  0
+#define KVM_X86_SW_PROTECTED_VM1
+#define KVM_X86_TDX_VM 2
+
+/* Trust Domain eXtension command*/
+enum tdx_cmd_id {
+   KVM_TDX_CAPABILITIES = 0,
+   KVM_TDX_INIT_VM,
+   KVM_TDX_INIT_VCPU,
+   KVM_TDX_INIT_MEM_REGION,
+   KVM_TDX_FINALIZE_VM,
+
+   KVM_TDX_CMD_NR_MAX,
+};
+
+struct kvm_tdx_cmd {
+   __u32 id;
+   __u32 metadata;
+   __u64 data;
+};
+
+struct kvm_tdx_cpuid_config {
+   __u32 leaf;
+   __u32 sub_leaf;
+   __u32 eax;
+   __u32 ebx;
+   __u32 ecx;
+   __u32 edx;
+};
+
+struct kvm_tdx_capabilities {
+   __u64 attrs_fixed0;
+   __u64 attrs_fixed1;
+   __u64 xfam_fixed0;
+   __u64 xfam_fixed1;
+
+   __u32 nr_cpuid_configs;
+   struct kvm_tdx_cpuid_config cpuid_configs[0];
+};
+
+struct kvm_tdx_init_vm {
+   __u32 max_vcpus;
+   __u32 reserved;
+   __u64 attributes;
+   __u64 cpuid;
+};
+
+#define KVM_TDX_MEASURE_MEMORY_REGION  (1UL << 0)
+
+struct kvm_tdx_init_mem_region {
+   __u64 source_addr;
+   __u64 gpa;
+   __u64 nr_pages;
+};
+
 #endif /* _ASM_X86_KVM_H */
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 020b62a619..0467c335a0 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1057,6 +1057,8 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_SYS_HYPERV_CPUID 191
 #define KVM_CAP_DIRTY_LOG_RING 192
 
+#define KVM_CAP_VM_TYPES 1000
+
 #ifdef KVM_CAP_IRQ_ROUTING
 
 struct kvm_irq_routing_irqchip {
-- 
2.17.1

[RFC PATCH 04/23] i386/kvm: Move architectural CPUID leaf generation to separarte helper

[Isaku Yamahata]

From: Sean Christopherson 

Move the architectural (for lack of a better term) CPUID leaf generation
to a separate helper so that the generation code can be reused by TDX,
which needs to generate a canonical VM-scoped configuration.

Signed-off-by: Sean Christopherson 
---
 target/i386/kvm/kvm.c  | 170 +++--
 target/i386/kvm/kvm_i386.h |   4 +
 2 files changed, 93 insertions(+), 81 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 8a04cf1337..bb241d8aa1 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1461,82 +1461,11 @@ static int hyperv_init_vcpu(X86CPU *cpu)
 
 static Error *invtsc_mig_blocker;
 
-#define KVM_MAX_CPUID_ENTRIES  100
-
-int kvm_arch_init_vcpu(CPUState *cs)
+uint32_t kvm_x86_arch_cpuid(CPUX86State *env, struct kvm_cpuid_entry2 *entries,
+uint32_t cpuid_i)
 {
-struct {
-struct kvm_cpuid2 cpuid;
-struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
-} cpuid_data;
-/*
- * The kernel defines these structs with padding fields so there
- * should be no extra padding in our cpuid_data struct.
- */
-QEMU_BUILD_BUG_ON(sizeof(cpuid_data) !=
-  sizeof(struct kvm_cpuid2) +
-  sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
-
-X86CPU *cpu = X86_CPU(cs);
-CPUX86State *env = >env;
-uint32_t limit, i, j, cpuid_i;
-uint32_t unused;
+uint32_t limit, i, j, unused;
 struct kvm_cpuid_entry2 *c;
-uint32_t signature[3];
-int kvm_base = KVM_CPUID_SIGNATURE;
-int max_nested_state_len;
-int r;
-Error *local_err = NULL;
-
-memset(_data, 0, sizeof(cpuid_data));
-
-cpuid_i = 0;
-
-r = kvm_arch_set_tsc_khz(cs);
-if (r < 0) {
-return r;
-}
-
-/* vcpu's TSC frequency is either specified by user, or following
- * the value used by KVM if the former is not present. In the
- * latter case, we query it from KVM and record in env->tsc_khz,
- * so that vcpu's TSC frequency can be migrated later via this field.
- */
-if (!env->tsc_khz) {
-r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
-kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) :
--ENOTSUP;
-if (r > 0) {
-env->tsc_khz = r;
-}
-}
-
-env->apic_bus_freq = KVM_APIC_BUS_FREQUENCY;
-
-/* Paravirtualization CPUIDs */
-r = hyperv_handle_properties(cs, cpuid_data.entries);
-if (r < 0) {
-return r;
-} else if (r > 0) {
-cpuid_i = r;
-kvm_base = KVM_CPUID_SIGNATURE_NEXT;
-has_msr_hv_hypercall = true;
-}
-
-if (cpu->expose_kvm) {
-memcpy(signature, "KVMKVMKVM\0\0\0", 12);
-c = _data.entries[cpuid_i++];
-c->function = KVM_CPUID_SIGNATURE | kvm_base;
-c->eax = KVM_CPUID_FEATURES | kvm_base;
-c->ebx = signature[0];
-c->ecx = signature[1];
-c->edx = signature[2];
-
-c = _data.entries[cpuid_i++];
-c->function = KVM_CPUID_FEATURES | kvm_base;
-c->eax = env->features[FEAT_KVM];
-c->edx = env->features[FEAT_KVM_HINTS];
-}
 
 cpu_x86_cpuid(env, 0, 0, , , , );
 
@@ -1545,7 +1474,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 fprintf(stderr, "unsupported level value: 0x%x\n", limit);
 abort();
 }
-c = _data.entries[cpuid_i++];
+c = [cpuid_i++];
 
 switch (i) {
 case 2: {
@@ -1564,7 +1493,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 "cpuid(eax:2):eax & 0xf = 0x%x\n", times);
 abort();
 }
-c = _data.entries[cpuid_i++];
+c = [cpuid_i++];
 c->function = i;
 c->flags = KVM_CPUID_FLAG_STATEFUL_FUNC;
 cpu_x86_cpuid(env, i, 0, >eax, >ebx, >ecx, >edx);
@@ -1610,7 +1539,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 "cpuid(eax:0x%x,ecx:0x%x)\n", i, j);
 abort();
 }
-c = _data.entries[cpuid_i++];
+c = [cpuid_i++];
 }
 break;
 case 0x7:
@@ -1629,7 +1558,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 "cpuid(eax:0x%x,ecx:0x%x)\n", i, j);
 abort();
 }
-c = _data.entries[cpuid_i++];
+c = [cpuid_i++];
 c->function = i;
 c->index = j;
 c->flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
@@ -1686,7 +1615,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 fprintf(stderr, "unsupported xlevel value: 0x%x\n", limit);
 abort();
 }
-c = _data.entries[cpuid_i++];
+c = [cpuid_i++];
 
 switch (i) {
 case 0x801d:
@@ -1705,7 +1634,7 @@ int kvm_arch_init_vcpu(CPUState *cs)

[RFC PATCH 17/23] i386/tdx: Add definitions for TDVF metadata

[Isaku Yamahata]

Add constants and structs for the TD Virtual Firmware metadata, which
describes how the TDVF must be built to ensure correct functionality and
measurement.

Signed-off-by: Isaku Yamahata 
Co-developed-by: Sean Christopherson 
Signed-off-by: Sean Christopherson 
---
 include/hw/i386/tdvf.h | 55 ++
 1 file changed, 55 insertions(+)
 create mode 100644 include/hw/i386/tdvf.h

diff --git a/include/hw/i386/tdvf.h b/include/hw/i386/tdvf.h
new file mode 100644
index 00..5c78e2affb
--- /dev/null
+++ b/include/hw/i386/tdvf.h
@@ -0,0 +1,55 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+
+ * Copyright (c) 2020 Intel Corporation
+ * Author: Isaku Yamahata 
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see .
+ */
+
+#ifndef HW_I386_TDVF_H
+#define HW_I386_TDVF_H
+
+#include "qemu/osdep.h"
+
+#define TDVF_METDATA_OFFSET_FROM_END0x20
+
+#define TDVF_SECTION_TYPE_BFV   0
+#define TDVF_SECTION_TYPE_CFV   1
+#define TDVF_SECTION_TYPE_TD_HOB2
+#define TDVF_SECTION_TYPE_TEMP_MEM  3
+
+#define TDVF_SECTION_ATTRIBUTES_EXTENDMR(1U << 0)
+
+typedef struct {
+uint32_t DataOffset;
+uint32_t RawDataSize;
+uint64_t MemoryAddress;
+uint64_t MemoryDataSize;
+uint32_t Type;
+uint32_t Attributes;
+} TdvfSectionEntry;
+
+#define TDVF_SIGNATURE_LE32 0x46564454 /* TDVF as little endian */
+
+typedef struct {
+uint8_t Signature[4];
+uint32_t Length;
+uint32_t Version;
+uint32_t NumberOfSectionEntries;
+TdvfSectionEntry SectionEntries[];
+} TdvfMetadata;
+
+#endif /* HW_I386_TDVF_H */
-- 
2.17.1

[RFC PATCH 09/23] target/i386: kvm: don't synchronize guest tsc for TD guest

[Isaku Yamahata]

Make kvm_synchronize_all_tsc() nop for TD-guest.

TDX module specification, 9.11.1 TSC Virtualization
"Virtual TSC values are consistent among all the TD;s VCPUs at the
level suppored by the CPU".
There is no need for qemu to synchronize tsc and VMM can't access
to guest TSC. Actually do_kvm_synchronize_tsc() hits assert due to
failure to write to guest tsc.

> qemu/target/i386/kvm.c:235: kvm_get_tsc: Assertion `ret == 1' failed.

Signed-off-by: Isaku Yamahata 
---
 target/i386/kvm/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index fb94cdd370..beb768a7d3 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -247,7 +247,7 @@ void kvm_synchronize_all_tsc(void)
 {
 CPUState *cpu;
 
-if (kvm_enabled()) {
+if (kvm_enabled() && vm_type != KVM_X86_TDX_VM) {
 CPU_FOREACH(cpu) {
 run_on_cpu(cpu, do_kvm_synchronize_tsc, RUN_ON_CPU_NULL);
 }
-- 
2.17.1

[RFC PATCH 08/23] i386/kvm: Skip KVM_X86_SETUP_MCE for TDX guests

[Isaku Yamahata]

Despite advertising MCE support to the guest, TDX-SEAM doesn't support
injecting #MCs into the guest.   All of the associated setup is thus
rejected by KVM.

Signed-off-by: Isaku Yamahata 
---
 target/i386/kvm/kvm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 9c5f669b7c..fb94cdd370 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1768,7 +1768,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
 if (((env->cpuid_version >> 8)&0xF) >= 6
 && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
(CPUID_MCE | CPUID_MCA)
-&& kvm_check_extension(cs->kvm_state, KVM_CAP_MCE) > 0) {
+&& kvm_check_extension(cs->kvm_state, KVM_CAP_MCE) > 0
+&& vm_type != KVM_X86_TDX_VM) {
 uint64_t mcg_cap, unsupported_caps;
 int banks;
 int ret;
-- 
2.17.1

[RFC PATCH 07/23] i386/kvm: Squash getting/putting guest state for TDX VMs

[Isaku Yamahata]

From: Sean Christopherson 

Ignore get/put state of TDX VMs as accessing/mutating guest state of
producation TDs is not supported.
Allow kvm_arch_get_registers() to run as normal, except for MSRs, for
debug TDs, and silently ignores attempts to read guest state for
non-debug TDs.

Signed-off-by: Sean Christopherson 
---
 target/i386/kvm/kvm.c | 18 +++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index ab7a896bd2..9c5f669b7c 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2565,6 +2565,11 @@ void kvm_put_apicbase(X86CPU *cpu, uint64_t value)
 {
 int ret;
 
+/* TODO: Allow accessing guest state for debug TDs. */
+if (vm_type == KVM_X86_TDX_VM) {
+return;
+}
+
 ret = kvm_put_one_msr(cpu, MSR_IA32_APICBASE, value);
 assert(ret == 1);
 }
@@ -4019,6 +4024,11 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
 
 assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
 
+/* TODO: Allow accessing guest state for debug TDs. */
+if (vm_type == KVM_X86_TDX_VM) {
+return 0;
+}
+
 /* must be before kvm_put_nested_state so that EFER.SVME is set */
 ret = kvm_put_sregs(x86_cpu);
 if (ret < 0) {
@@ -4129,9 +4139,11 @@ int kvm_arch_get_registers(CPUState *cs)
 if (ret < 0) {
 goto out;
 }
-ret = kvm_get_msrs(cpu);
-if (ret < 0) {
-goto out;
+if (vm_type != KVM_X86_TDX_VM) {
+ret = kvm_get_msrs(cpu);
+if (ret < 0) {
+goto out;
+}
 }
 ret = kvm_get_apic(cpu);
 if (ret < 0) {
-- 
2.17.1

[RFC PATCH 12/23] target/i386/tdx: Finalize the TD's measurement when machine is done

[Isaku Yamahata]

From: Xiaoyao Li 

Invoke KVM_TDX_FINALIZEMR to finalize the TD's measurement and make the
TD vCPUs runnable once machine initialization is complete.

Signed-off-by: Xiaoyao Li 
---
 target/i386/kvm/kvm.c |  7 +++
 target/i386/kvm/tdx.c | 20 
 target/i386/kvm/tdx.h |  3 +++
 3 files changed, 30 insertions(+)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index beb768a7d3..018a757dc6 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -51,6 +51,7 @@
 #include "migration/blocker.h"
 #include "exec/memattrs.h"
 #include "trace.h"
+#include "tdx.h"
 
 //#define DEBUG_KVM
 
@@ -2184,6 +2185,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
 return ret;
 }
 
+ret = tdx_kvm_init(ms->cgs, _err);
+if (ret < 0) {
+error_report_err(local_err);
+return ret;
+}
+
 if (!kvm_check_extension(s, KVM_CAP_IRQ_ROUTING)) {
 error_report("kvm: KVM_CAP_IRQ_ROUTING not supported by KVM");
 return -ENOTSUP;
diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index 00eda80725..d8b79e975f 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -56,6 +56,26 @@ static void __tdx_ioctl(int ioctl_no, const char *ioctl_name,
 #define tdx_ioctl(ioctl_no, metadata, data) \
 __tdx_ioctl(ioctl_no, stringify(ioctl_no), metadata, data)
 
+static void tdx_finalize_vm(Notifier *notifier, void *unused)
+{
+tdx_ioctl(KVM_TDX_FINALIZE_VM, 0, NULL);
+}
+
+static Notifier tdx_machine_done_late_notify = {
+.notify = tdx_finalize_vm,
+};
+
+int tdx_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
+{
+TdxGuest *tdx = (TdxGuest *)object_dynamic_cast(OBJECT(cgs),
+TYPE_TDX_GUEST);
+if (tdx) {
+qemu_add_machine_init_done_late_notifier(
+_machine_done_late_notify);
+}
+return 0;
+}
+
 void tdx_pre_create_vcpu(CPUState *cpu)
 {
 struct {
diff --git a/target/i386/kvm/tdx.h b/target/i386/kvm/tdx.h
index 6ad6c9a313..e15657d272 100644
--- a/target/i386/kvm/tdx.h
+++ b/target/i386/kvm/tdx.h
@@ -2,6 +2,7 @@
 #define QEMU_I386_TDX_H
 
 #include "qom/object.h"
+#include "qapi/error.h"
 #include "exec/confidential-guest-support.h"
 
 #define TYPE_TDX_GUEST "tdx-guest"
@@ -21,4 +22,6 @@ typedef struct TdxGuest {
 bool debug;
 } TdxGuest;
 
+int tdx_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
+
 #endif
-- 
2.17.1

[RFC PATCH 03/23] KVM: i386: use VM capability check for KVM_CAP_X86_SMM

[Isaku Yamahata]

From: Isaku Yamahata 

KVM_CAP_X86_SMM is VM-specific capability for TDX.
So use VM ioctl for KVM_CAP_X86_SMM.

Signed-off-by: Isaku Yamahata 
---
 target/i386/kvm/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index e97f841757..8a04cf1337 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -136,7 +136,7 @@ int kvm_has_pit_state2(void)
 
 bool kvm_has_smm(void)
 {
-return kvm_check_extension(kvm_state, KVM_CAP_X86_SMM);
+return kvm_vm_check_extension(kvm_state, KVM_CAP_X86_SMM);
 }
 
 bool kvm_has_adjust_clock_stable(void)
-- 
2.17.1

[RFC PATCH 02/23] kvm: Switch KVM_CAP_READONLY_MEM to a per-VM ioctl()

[Isaku Yamahata]

Switch to making a VM ioctl() call for KVM_CAP_READONLY_MEM, which may
be conditional on VM type in recent versions of KVM, e.g. when TDX is
supported.

Signed-off-by: Isaku Yamahata 
---
 accel/kvm/kvm-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 47516913b7..351c25a5cb 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2164,7 +2164,7 @@ static int kvm_init(MachineState *ms)
 }
 
 kvm_readonly_mem_allowed =
-(kvm_check_extension(s, KVM_CAP_READONLY_MEM) > 0);
+(kvm_vm_check_extension(s, KVM_CAP_READONLY_MEM) > 0);
 
 kvm_eventfds_allowed =
 (kvm_check_extension(s, KVM_CAP_IOEVENTFD) > 0);
-- 
2.17.1

[RFC PATCH 06/23] hw/i386: Introduce kvm-type for TDX guest

[Isaku Yamahata]

From: Xiaoyao Li 

Introduce a machine property, kvm-type, to allow the user to create a
Trusted Domain eXtensions (TDX) VM, a.k.a. a Trusted Domain (TD), e.g.:

 # $QEMU \
-machine ...,kvm-type=tdx \
...

Only two types are supported: "legacy" and "tdx", with "legacy" being
the default.

Signed-off-by: Xiaoyao Li 
Co-developed-by: Sean Christopherson 
Signed-off-by: Sean Christopherson 
---
 default-configs/devices/i386-softmmu.mak |  1 +
 hw/i386/Kconfig  |  5 +++
 hw/i386/x86.c| 46 
 include/hw/i386/x86.h|  1 +
 include/sysemu/tdx.h | 10 ++
 target/i386/kvm/kvm-stub.c   |  5 +++
 target/i386/kvm/kvm.c| 15 
 target/i386/kvm/kvm_i386.h   |  1 +
 target/i386/kvm/meson.build  |  1 +
 target/i386/kvm/tdx-stub.c   | 10 ++
 target/i386/kvm/tdx.c| 30 
 11 files changed, 125 insertions(+)
 create mode 100644 include/sysemu/tdx.h
 create mode 100644 target/i386/kvm/tdx-stub.c
 create mode 100644 target/i386/kvm/tdx.c

diff --git a/default-configs/devices/i386-softmmu.mak 
b/default-configs/devices/i386-softmmu.mak
index 84d1a2487c..6e805407b8 100644
--- a/default-configs/devices/i386-softmmu.mak
+++ b/default-configs/devices/i386-softmmu.mak
@@ -18,6 +18,7 @@
 #CONFIG_QXL=n
 #CONFIG_SEV=n
 #CONFIG_SGA=n
+#CONFIG_TDX=n
 #CONFIG_TEST_DEVICES=n
 #CONFIG_TPM_CRB=n
 #CONFIG_TPM_TIS_ISA=n
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index 7f91f30877..bc79e1e84a 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -2,6 +2,10 @@ config SEV
 bool
 depends on KVM
 
+config TDX
+bool
+depends on KVM
+
 config PC
 bool
 imply APPLESMC
@@ -17,6 +21,7 @@ config PC
 imply PVPANIC_ISA
 imply QXL
 imply SEV
+imply TDX
 imply SGA
 imply TEST_DEVICES
 imply TPM_CRB
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 6329f90ef9..a4a0cc83dd 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -21,6 +21,7 @@
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
+#include 
 #include "qemu/error-report.h"
 #include "qemu/option.h"
 #include "qemu/cutils.h"
@@ -31,6 +32,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qapi-visit-common.h"
 #include "qapi/visitor.h"
+#include "sysemu/kvm_int.h"
 #include "sysemu/qtest.h"
 #include "sysemu/whpx.h"
 #include "sysemu/numa.h"
@@ -1199,6 +1201,43 @@ static void x86_machine_set_acpi(Object *obj, Visitor 
*v, const char *name,
 visit_type_OnOffAuto(v, name, >acpi, errp);
 }
 
+static char *x86_get_kvm_type(Object *obj, Error **errp)
+{
+X86MachineState *x86ms = X86_MACHINE(obj);
+
+return g_strdup(x86ms->kvm_type);
+}
+
+
+static void x86_set_kvm_type(Object *obj, const char *value, Error **errp)
+{
+X86MachineState *x86ms = X86_MACHINE(obj);
+
+g_free(x86ms->kvm_type);
+x86ms->kvm_type = g_strdup(value);
+}
+
+static int x86_kvm_type(MachineState *ms, const char *vm_type)
+{
+int kvm_type;
+
+if (!vm_type || !strcmp(vm_type, "") ||
+!g_ascii_strcasecmp(vm_type, "legacy")) {
+kvm_type = KVM_X86_LEGACY_VM;
+} else if (!g_ascii_strcasecmp(vm_type, "tdx")) {
+kvm_type = KVM_X86_TDX_VM;
+} else {
+error_report("Unknown kvm-type specified '%s'", vm_type);
+exit(1);
+}
+if (kvm_set_vm_type(ms, kvm_type)) {
+error_report("kvm-type '%s' not supported by KVM", vm_type);
+exit(1);
+}
+
+return kvm_type;
+}
+
 static void x86_machine_initfn(Object *obj)
 {
 X86MachineState *x86ms = X86_MACHINE(obj);
@@ -1207,6 +1246,12 @@ static void x86_machine_initfn(Object *obj)
 x86ms->acpi = ON_OFF_AUTO_AUTO;
 x86ms->smp_dies = 1;
 x86ms->pci_irq_mask = ACPI_BUILD_PCI_IRQS;
+
+
+object_property_add_str(obj, "kvm-type",
+x86_get_kvm_type, x86_set_kvm_type);
+object_property_set_description(obj, "kvm-type",
+"KVM guest type (legacy, tdx)");
 }
 
 static void x86_machine_class_init(ObjectClass *oc, void *data)
@@ -1218,6 +1263,7 @@ static void x86_machine_class_init(ObjectClass *oc, void 
*data)
 mc->cpu_index_to_instance_props = x86_cpu_index_to_props;
 mc->get_default_cpu_node_id = x86_get_default_cpu_node_id;
 mc->possible_cpu_arch_ids = x86_possible_cpu_arch_ids;
+mc->kvm_type = x86_kvm_type;
 x86mc->compat_apic_id_mode = false;
 x86mc->save_tsc_khz = true;
 nc->nmi_monitor_handler = x86_nmi;
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 56080bd1fb..05e3b738d1 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -56,6 +56,7 @@ struct X86MachineState {
 
 /* RAM information (sizes, addresses, configuration): */
 ram_addr_t below_4g_mem_size, above_4g_mem_size;
+char *kvm_type;
 
 /* CPU and apic information: */
 bool apic_xrupt_override;
diff --git 

[RFC PATCH 05/23] vl: Introduce machine_init_done_late notifier

[Isaku Yamahata]

Introduce a new notifier, machine_init_done_late, that is notified after
machine_init_done.  This will be used by TDX to generate the HOB for its
virtual firmware, which needs to be done after all guest memory has been
added, i.e. after machine_init_done notifiers have run.
some devices adds/updates memory region by machine_init_done notifier.

Signed-off-by: Isaku Yamahata 
---
 hw/core/machine.c   | 26 ++
 include/sysemu/sysemu.h |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 970046f438..02be08f3d1 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1231,6 +1231,31 @@ void qemu_remove_machine_init_done_notifier(Notifier 
*notify)
 notifier_remove(notify);
 }
 
+static NotifierList machine_init_done_late_notifiers =
+NOTIFIER_LIST_INITIALIZER(machine_init_done_late_notifiers);
+
+static bool machine_init_done_late;
+
+void qemu_add_machine_init_done_late_notifier(Notifier *notify)
+{
+notifier_list_add(_init_done_late_notifiers, notify);
+if (machine_init_done_late) {
+notify->notify(notify, NULL);
+}
+}
+
+void qemu_remove_machine_init_done_late_notifier(Notifier *notify)
+{
+notifier_remove(notify);
+}
+
+
+static void qemu_run_machine_init_done_late_notifiers(void)
+{
+machine_init_done_late = true;
+notifier_list_notify(_init_done_late_notifiers, NULL);
+}
+
 void qdev_machine_creation_done(void)
 {
 cpu_synchronize_all_post_init();
@@ -1264,6 +1289,7 @@ void qdev_machine_creation_done(void)
 if (rom_check_and_register_reset() != 0) {
 exit(1);
 }
+qemu_run_machine_init_done_late_notifiers();
 
 replay_start();
 
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 8fae667172..d44f8cf778 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -19,6 +19,8 @@ void qemu_remove_exit_notifier(Notifier *notify);
 void qemu_run_machine_init_done_notifiers(void);
 void qemu_add_machine_init_done_notifier(Notifier *notify);
 void qemu_remove_machine_init_done_notifier(Notifier *notify);
+void qemu_add_machine_init_done_late_notifier(Notifier *notify);
+void qemu_remove_machine_init_done_late_notifier(Notifier *notify);
 
 void configure_rtc(QemuOpts *opts);
 
-- 
2.17.1

[RFC PATCH 00/23] [RFC PATCH 00/24] TDX support

[Isaku Yamahata]

This patch series is to enable TDX support.
This needs corresponding KVM patch for TDX[] and more patches are needed
that addresses generic corner cases, e.g. ACPI related stuff, are needed.
So This patch series is RFC.
More emulated devices and their behavior needs to be adjusted as some
operations are disallowed.

Patch series is organized as follows
 1- 5 code refactoring and simple hooks that will be used later
 6- 9 introduce kvm type and tdx type. disallow non-usable operations
10-15 wire up necessary TDX kvm ioctl to initialize TD guest
16-21 load TDVF and setup necessary info for TDVF
22-23 force x2apic and disable PIC

Isaku Yamahata (12):
  kvm: Switch KVM_CAP_READONLY_MEM to a per-VM ioctl()
  KVM: i386: use VM capability check for KVM_CAP_X86_SMM
  vl: Introduce machine_init_done_late notifier
  i386/kvm: Skip KVM_X86_SETUP_MCE for TDX guests
  target/i386: kvm: don't synchronize guest tsc for TD guest
  i386/tdx: Frame in the call for KVM_TDX_INIT_VCPU
  hw/i386: Add definitions from UEFI spec for volumes, resources, etc...
  i386/tdx: Add definitions for TDVF metadata
  i386/tdx: Parse tdvf metadata and store the result into TdxGuest
  i386/tdx: Create the TD HOB list upon machine init done
  i386/tdx: Add TDVF memory via INIT_MEM_REGION
  i386/tdx: Use KVM_TDX_INIT_VCPU to pass HOB to TDVF

Sean Christopherson (7):
  target/i386: Expose x86_cpu_get_supported_feature_word() for TDX
  i386/kvm: Move architectural CPUID leaf generation to separarte helper
  i386/kvm: Squash getting/putting guest state for TDX VMs
  i386/tdx: Frame in tdx_get_supported_cpuid with KVM_TDX_CAPABILITIES
  i386/tdx: Add hook to require generic device loader
  i386/tdx: Force x2apic mode and routing for TDs
  target/i386: Add machine option to disable PIC/8259

Xiaoyao Li (4):
  hw/i386: Introduce kvm-type for TDX guest
  linux-headers: Update headers to pull in TDX API changes
  hw/i386: Initialize TDX via KVM ioctl() when kvm_type is TDX
  target/i386/tdx: Finalize the TD's measurement when machine is done

 accel/kvm/kvm-all.c  |   4 +-
 default-configs/devices/i386-softmmu.mak |   1 +
 hw/core/generic-loader.c |   5 +
 hw/core/machine.c|  26 ++
 hw/core/meson.build  |   3 +
 hw/core/tdvf-stub.c  |   6 +
 hw/i386/Kconfig  |   5 +
 hw/i386/meson.build  |   1 +
 hw/i386/pc.c |  18 +
 hw/i386/pc_piix.c|   4 +-
 hw/i386/pc_q35.c |   4 +-
 hw/i386/pc_sysfw.c   |   6 +
 hw/i386/tdvf-hob.c   | 226 +++
 hw/i386/tdvf-hob.h   |  25 ++
 hw/i386/tdvf.c   | 305 ++
 hw/i386/uefi.h   | 496 +++
 hw/i386/x86.c|  46 +++
 hw/intc/apic_common.c|  12 +
 include/hw/i386/apic.h   |   1 +
 include/hw/i386/apic_internal.h  |   1 +
 include/hw/i386/pc.h |   2 +
 include/hw/i386/tdvf.h   |  55 +++
 include/hw/i386/x86.h|   1 +
 include/sysemu/sysemu.h  |   2 +
 include/sysemu/tdvf.h|   6 +
 include/sysemu/tdx.h |  15 +
 linux-headers/asm-x86/kvm.h  |  55 +++
 linux-headers/linux/kvm.h|   2 +
 target/i386/cpu.c|   4 +-
 target/i386/cpu.h|   3 +
 target/i386/kvm/kvm-stub.c   |   5 +
 target/i386/kvm/kvm.c| 227 +++
 target/i386/kvm/kvm_i386.h   |   5 +
 target/i386/kvm/meson.build  |   1 +
 target/i386/kvm/tdx-stub.c   |  23 ++
 target/i386/kvm/tdx.c| 329 +++
 target/i386/kvm/tdx.h|  55 +++
 37 files changed, 1893 insertions(+), 92 deletions(-)
 create mode 100644 hw/core/tdvf-stub.c
 create mode 100644 hw/i386/tdvf-hob.c
 create mode 100644 hw/i386/tdvf-hob.h
 create mode 100644 hw/i386/tdvf.c
 create mode 100644 hw/i386/uefi.h
 create mode 100644 include/hw/i386/tdvf.h
 create mode 100644 include/sysemu/tdvf.h
 create mode 100644 include/sysemu/tdx.h
 create mode 100644 target/i386/kvm/tdx-stub.c
 create mode 100644 target/i386/kvm/tdx.c
 create mode 100644 target/i386/kvm/tdx.h

-- 
2.17.1

[PATCH] hw/char: disable ibex uart receive if the buffer is full

[Alexander Wagner]

Not disabling the UART leads to QEMU overwriting the UART receive buffer with
the newest received byte. The rx_level variable is added to allow the use of
the existing OpenTitan driver libraries.

Signed-off-by: Alexander Wagner 
---
 hw/char/ibex_uart.c | 20 +++-
 include/hw/char/ibex_uart.h |  4 
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
index 89f1182c9b..dac09d53d6 100644
--- a/hw/char/ibex_uart.c
+++ b/hw/char/ibex_uart.c
@@ -66,7 +66,8 @@ static int ibex_uart_can_receive(void *opaque)
 {
 IbexUartState *s = opaque;
 
-if (s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) {
+if ((s->uart_ctrl & R_CTRL_RX_ENABLE_MASK)
+   && !(s->uart_status & R_STATUS_RXFULL_MASK)) {
 return 1;
 }
 
@@ -83,6 +84,8 @@ static void ibex_uart_receive(void *opaque, const uint8_t 
*buf, int size)
 
 s->uart_status &= ~R_STATUS_RXIDLE_MASK;
 s->uart_status &= ~R_STATUS_RXEMPTY_MASK;
+s->uart_status |= R_STATUS_RXFULL_MASK;
+s->rx_level += 1;
 
 if (size > rx_fifo_level) {
 s->uart_intr_state |= R_INTR_STATE_RX_WATERMARK_MASK;
@@ -199,6 +202,7 @@ static void ibex_uart_reset(DeviceState *dev)
 s->uart_timeout_ctrl = 0x;
 
 s->tx_level = 0;
+s->rx_level = 0;
 
 s->char_tx_time = (NANOSECONDS_PER_SECOND / 230400) * 10;
 
@@ -243,11 +247,15 @@ static uint64_t ibex_uart_read(void *opaque, hwaddr addr,
 
 case R_RDATA:
 retvalue = s->uart_rdata;
-if (s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) {
+if ((s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) && (s->rx_level > 0)) {
 qemu_chr_fe_accept_input(>chr);
 
-s->uart_status |= R_STATUS_RXIDLE_MASK;
-s->uart_status |= R_STATUS_RXEMPTY_MASK;
+s->rx_level -= 1;
+s->uart_status &= ~R_STATUS_RXFULL_MASK;
+if (s->rx_level == 0) {
+s->uart_status |= R_STATUS_RXIDLE_MASK;
+s->uart_status |= R_STATUS_RXEMPTY_MASK;
+}
 }
 break;
 case R_WDATA:
@@ -261,7 +269,8 @@ static uint64_t ibex_uart_read(void *opaque, hwaddr addr,
 case R_FIFO_STATUS:
 retvalue = s->uart_fifo_status;
 
-retvalue |= s->tx_level & 0x1F;
+retvalue |= (s->rx_level & 0x1F) << R_FIFO_STATUS_RXLVL_SHIFT;
+retvalue |= (s->tx_level & 0x1F) << R_FIFO_STATUS_TXLVL_SHIFT;
 
 qemu_log_mask(LOG_UNIMP,
   "%s: RX fifos are not supported\n", __func__);
@@ -364,6 +373,7 @@ static void ibex_uart_write(void *opaque, hwaddr addr,
 s->uart_fifo_ctrl = value;
 
 if (value & R_FIFO_CTRL_RXRST_MASK) {
+s->rx_level = 0;
 qemu_log_mask(LOG_UNIMP,
   "%s: RX fifos are not supported\n", __func__);
 }
diff --git a/include/hw/char/ibex_uart.h b/include/hw/char/ibex_uart.h
index 03d19e3f6f..546f958eb8 100644
--- a/include/hw/char/ibex_uart.h
+++ b/include/hw/char/ibex_uart.h
@@ -62,6 +62,8 @@ REG32(FIFO_CTRL, 0x1c)
 FIELD(FIFO_CTRL, RXILVL, 2, 3)
 FIELD(FIFO_CTRL, TXILVL, 5, 2)
 REG32(FIFO_STATUS, 0x20)
+FIELD(FIFO_STATUS, TXLVL, 0, 5)
+FIELD(FIFO_STATUS, RXLVL, 16, 5)
 REG32(OVRD, 0x24)
 REG32(VAL, 0x28)
 REG32(TIMEOUT_CTRL, 0x2c)
@@ -82,6 +84,8 @@ struct IbexUartState {
 uint8_t tx_fifo[IBEX_UART_TX_FIFO_SIZE];
 uint32_t tx_level;
 
+uint32_t rx_level;
+
 QEMUTimer *fifo_trigger_handle;
 uint64_t char_tx_time;
 
-- 
2.25.1

Re: [PATCH v9 0/6] Rework iotests/check

[John Snow]

On 1/26/21 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:




OK, thanks for handling it!

When will we move to python 3.7?


"I don't know, but it seems like a very long time."

The nominal EOL for Python 3.6 is this December; see 
https://www.python.org/dev/peps/pep-0494/


Debian 10 ships 3.7.
Fedora has appropriate packages going back to F29 and possibly earlier.
OpenSuSE tumbleweed offers 3.8 and 3.9.
Ubuntu 18.04 LTS offers 3.7 and 3.8 packages.
Ubuntu 20.04 LTS offers 3.7 and 3.8 packages.

OpenSUSE 15.2 ... ships with Python 3.6, and I don't see an option to 
install anything more modern. This distribution is supported (both by us 
and by SuSE) until 2021-12-31.


As of 2020-12-29, OpenSuSE 15.3 appears to be set to ship Python 3.6:
"python3-3.6.12-3.70.1.x86_64.rpm". It is set to ship 2021-07-07, and 
does not have an estimated EOL date yet.


SLES 15 ... I'm not sure and can't seem to find that information 
quickly. https://www.suse.com/releasenotes/x86_64/SUSE-SLES/15-SP1/ 
which was published at the end of 2020, suggests that Python 3 
support(?) is new(?) to some extent. 
https://packagehub.suse.com/packages/python3/ suggests that SLES 15.2 is 
Python 3.6-based.


I can't tell if they offer the optional addition of Python 3.7 on either 
SLES or OpenSuSE. Would love to know.


As for RHEL/CentOS, I think it's in the same shape right now. It's 
3.6-based, but I don't know if there's an optional 3.7+ package or not.


--js


Note: our configure script doesn't try several canonical sources of 
Python interpreters, we just try to use "python3". In the case of 
multiple python installations, we'd have to try python3.7, python3.8, 
python3.9, etc.

Re: [PATCH 0/4] hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409

[Alexander Bulekov]

On 210216 0855, Bin Meng wrote:
> Hi Alex,
> 
> On Tue, Feb 16, 2021 at 12:48 AM Alexander Bulekov  wrote:
> >
> > Hi Bin,
> > Thank you for this. I ran through the OSS-Fuzz tests again, and it found
> > one thing:
> 
> Thanks for testing. Are there instructions to run OSS-Fuzz tests myself?

Yes we have some documentation in docs/devel/fuzzing.rst, but it
doesn't talk about using the OSS-Fuzz corpus.  The OSS-Fuzz corpus is
private, by default, but I uploaded a copy of the current sdhci corpus
here:
https://drive.google.com/file/d/1PcwFbY9YXPdaJ3aapIV2BI-bN5mbBgif/view?usp=sharing

To build the fuzzer, you need clang:

build the fuzzers
$ CC=clang CXX=clang++ ../configure --enable-fuzzing --enable-sanitizers \
--disable-werror
$ ninja -j`nproc` qemu-fuzz-i386

untar the corpus somewhere (~300 MB uncompressed)
$ tar -xvf sdhci-corpus.tar.gz

run through all the inputs once
$ ./qemu-fuzz-i386 --fuzz-target=generic-fuzz-sdhci-v3 \
  ~/path/to/corpus/qemu_qemu-fuzz-i386-target-generic-fuzz-sdhci-v3/* &> out

That will take some minutes, but you can look at the out file and search
for "ERROR" to find crashing inputs. 

-Alex
> 
> > Maybe this is already much better than the current state of the code, so
> > this one can be fixed in a later patch?
> 
> Depend on when Philippe can pick up this sereis, but I can also try to
> have a quick look :)
> 
> >
> > cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest \
> > -m 512M -nodefaults -device sdhci-pci,sd-spec-version=3 \
> > -device sd-card,drive=mydrive \
> > -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
> > -nographic -qtest stdio
> > outl 0xcf8 0x80001010
> > outl 0xcfc 0xe000
> > outl 0xcf8 0x80001001
> > outl 0xcfc 0x0600
> > write 0xe02c 0x1 0x05
> > write 0xe005 0x1 0x02
> > write 0xe007 0x1 0x01
> > write 0xe028 0x1 0x10
> > write 0x0 0x1 0x23
> > write 0x2 0x1 0x08
> > write 0xe00c 0x1 0x01
> > write 0xe00e 0x1 0x20
> > write 0xe00f 0x1 0x00
> > write 0xe00c 0x1 0x32
> > write 0xe004 0x2 0x0200
> > write 0xe028 0x1 0x00
> > write 0xe003 0x1 0x40
> > EOF
> >
> >
> > ==1730971==ERROR: AddressSanitizer: heap-buffer-overflow on address
> > 0x61531880 at pc 0x55d070f2c6d9 bp 0x7ffdcb63f130 sp 0x7ffdcb63f128
> > READ of size 4 at 0x61531880 thread T0
> > #0 0x55d070f2c6d8 in ldl_he_p bswap.h:347:5
> > #1 0x55d070f2c6d8 in ldn_he_p bswap.h:546:1
> > #2 0x55d070f2c6d8 in flatview_write_continue 
> > build/../softmmu/physmem.c:2775:19
> > #3 0x55d070f219eb in flatview_write build/../softmmu/physmem.c:2816:14
> > #4 0x55d070f219eb in address_space_write build/../softmmu/physmem.c:2908:18
> > #5 0x55d07040de4a in dma_memory_rw_relaxed include/sysemu/dma.h:88:12
> > #6 0x55d07040de4a in dma_memory_rw include/sysemu/dma.h:127:12
> > #7 0x55d07040de4a in dma_memory_write include/sysemu/dma.h:163:12
> > #8 0x55d07040de4a in sdhci_sdma_transfer_multi_blocks 
> > build/../hw/sd/sdhci.c:619:13
> > #9 0x55d07041d15b in sdhci_write build/../hw/sd/sdhci.c:1134:21
> > #10 0x55d07123b1ac in memory_region_write_accessor 
> > build/../softmmu/memory.c:491:5
> > #11 0x55d07123acab in access_with_adjusted_size 
> > build/../softmmu/memory.c:552:18
> > #12 0x55d07123a4b0 in memory_region_dispatch_write build/../softmmu/memory.c
> > #13 0x55d070f2c29b in flatview_write_continue 
> > build/../softmmu/physmem.c:2776:23
> > #14 0x55d070f219eb in flatview_write build/../softmmu/physmem.c:2816:14
> > #15 0x55d070f219eb in address_space_write build/../softmmu/physmem.c:2908:18
> 
> Regards,
> Bin

[PATCH v4 03/10] ich9, piix4: add properoty, smm-compat, to keep compatibility of SMM

[isaku . yamahata]

From: Isaku Yamahata 

The following patch will introduce incompatible behavior of SMM.
Introduce a property to keep the old behavior for compatibility.
To enable smm compat, use "-global ICH9-LPC.smm-compat=on" or
"-global PIIX4.smm-compat=on"

Suggested-by: Igor Mammedov 
Signed-off-by: Isaku Yamahata 
---
 hw/acpi/piix4.c| 2 ++
 hw/core/machine.c  | 5 -
 hw/isa/lpc_ich9.c  | 1 +
 include/hw/acpi/ich9.h | 1 +
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 669be5bbf6..30dd9b2309 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -74,6 +74,7 @@ struct PIIX4PMState {
 qemu_irq irq;
 qemu_irq smi_irq;
 int smm_enabled;
+bool smm_compat;
 Notifier machine_ready;
 Notifier powerdown_notifier;
 
@@ -642,6 +643,7 @@ static Property piix4_pm_properties[] = {
  use_acpi_root_pci_hotplug, true),
 DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
  acpi_memory_hotplug.is_enabled, true),
+DEFINE_PROP_BOOL("smm-compat", PIIX4PMState, smm_compat, false),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index de3b8f1b31..870c9201df 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -33,7 +33,10 @@
 #include "migration/global_state.h"
 #include "migration/vmstate.h"
 
-GlobalProperty hw_compat_5_2[] = {};
+GlobalProperty hw_compat_5_2[] = {
+{ "ICH9-LPC", "smm-compat", "on"},
+{ "PIIX4_PM", "smm-compat", "on"},
+};
 const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
 
 GlobalProperty hw_compat_5_1[] = {
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index d3145bf014..3963b73520 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -775,6 +775,7 @@ static const VMStateDescription vmstate_ich9_lpc = {
 
 static Property ich9_lpc_properties[] = {
 DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
+DEFINE_PROP_BOOL("smm-compat", ICH9LPCState, pm.smm_compat, false),
 DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
   ICH9_LPC_SMI_F_BROADCAST_BIT, true),
 DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 54571c77e0..df519e40b5 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -59,6 +59,7 @@ typedef struct ICH9LPCPMRegs {
 uint8_t disable_s4;
 uint8_t s4_val;
 uint8_t smm_enabled;
+bool smm_compat;
 bool enable_tco;
 TCOIORegs tco_regs;
 } ICH9LPCPMRegs;
-- 
2.17.1

[PATCH v4 04/10] acpi/core: always set SCI_EN when SMM isn't supported

[isaku . yamahata]

From: Isaku Yamahata 

If SMM is not supported, ACPI fixed hardware doesn't support
legacy-mode. ACPI-only platform. Where SCI_EN in PM1_CNT register is
always set.
The bit tells OS legacy mode(SCI_EN cleared) or ACPI mode(SCI_EN set).

With the next patch (setting fadt.smi_cmd = 0 when smm isn't enabled),
guest Linux tries to switch to ACPI mode, finds smi_cmd = 0, and then
fails to initialize acpi subsystem. This patch proactively fixes it.

This patch changes guest ABI. To keep compatibility, use
"x-smm-compat-5" introduced by earlier patch.
If the property is true, disable new behavior.

ACPI spec 4.8.10.1 PM1 Event Grouping
PM1 Eanble Registers
> For ACPI-only platforms (where SCI_EN is always set)

Signed-off-by: Isaku Yamahata 
---
 hw/acpi/core.c | 11 ++-
 hw/acpi/ich9.c |  2 +-
 hw/acpi/piix4.c|  3 ++-
 hw/isa/vt82c686.c  |  2 +-
 include/hw/acpi/acpi.h |  4 +++-
 5 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 7170bff657..1e004d0078 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -579,6 +579,10 @@ void acpi_pm1_cnt_update(ACPIREGS *ar,
  bool sci_enable, bool sci_disable)
 {
 /* ACPI specs 3.0, 4.7.2.5 */
+if (ar->pm1.cnt.acpi_only) {
+return;
+}
+
 if (sci_enable) {
 ar->pm1.cnt.cnt |= ACPI_BITMASK_SCI_ENABLE;
 } else if (sci_disable) {
@@ -608,11 +612,13 @@ static const MemoryRegionOps acpi_pm_cnt_ops = {
 };
 
 void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
-   bool disable_s3, bool disable_s4, uint8_t s4_val)
+   bool disable_s3, bool disable_s4, uint8_t s4_val,
+   bool acpi_only)
 {
 FWCfgState *fw_cfg;
 
 ar->pm1.cnt.s4_val = s4_val;
+ar->pm1.cnt.acpi_only = acpi_only;
 ar->wakeup.notify = acpi_notify_wakeup;
 qemu_register_wakeup_notifier(>wakeup);
 
@@ -638,6 +644,9 @@ void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
 void acpi_pm1_cnt_reset(ACPIREGS *ar)
 {
 ar->pm1.cnt.cnt = 0;
+if (ar->pm1.cnt.acpi_only) {
+ar->pm1.cnt.cnt |= ACPI_BITMASK_SCI_ENABLE;
+}
 }
 
 /* ACPI GPE */
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 5ff4e01c36..853447cf9d 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -282,7 +282,7 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
 acpi_pm_tmr_init(>acpi_regs, ich9_pm_update_sci_fn, >io);
 acpi_pm1_evt_init(>acpi_regs, ich9_pm_update_sci_fn, >io);
 acpi_pm1_cnt_init(>acpi_regs, >io, pm->disable_s3, pm->disable_s4,
-  pm->s4_val);
+  pm->s4_val, !pm->smm_compat && !smm_enabled);
 
 acpi_gpe_init(>acpi_regs, ICH9_PMIO_GPE0_LEN);
 memory_region_init_io(>io_gpe, OBJECT(lpc_pci), _gpe_ops, pm,
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 30dd9b2309..1efc0ded9f 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -497,7 +497,8 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
 
 acpi_pm_tmr_init(>ar, pm_tmr_timer, >io);
 acpi_pm1_evt_init(>ar, pm_tmr_timer, >io);
-acpi_pm1_cnt_init(>ar, >io, s->disable_s3, s->disable_s4, s->s4_val);
+acpi_pm1_cnt_init(>ar, >io, s->disable_s3, s->disable_s4, s->s4_val,
+  !s->smm_compat && !s->smm_enabled);
 acpi_gpe_init(>ar, GPE_LEN);
 
 s->powerdown_notifier.notify = piix4_pm_powerdown_req;
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index a6f5a0843d..071b64b497 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -240,7 +240,7 @@ static void vt82c686b_pm_realize(PCIDevice *dev, Error 
**errp)
 
 acpi_pm_tmr_init(>ar, pm_tmr_timer, >io);
 acpi_pm1_evt_init(>ar, pm_tmr_timer, >io);
-acpi_pm1_cnt_init(>ar, >io, false, false, 2);
+acpi_pm1_cnt_init(>ar, >io, false, false, 2, false);
 }
 
 static Property via_pm_properties[] = {
diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index 22b0b65bb2..9e8a76f2e2 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -128,6 +128,7 @@ struct ACPIPM1CNT {
 MemoryRegion io;
 uint16_t cnt;
 uint8_t s4_val;
+bool acpi_only;
 };
 
 struct ACPIGPE {
@@ -163,7 +164,8 @@ void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn 
update_sci,
 
 /* PM1a_CNT: piix and ich9 don't implement PM1b CNT. */
 void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
-   bool disable_s3, bool disable_s4, uint8_t s4_val);
+   bool disable_s3, bool disable_s4, uint8_t s4_val,
+   bool acpi_only);
 void acpi_pm1_cnt_update(ACPIREGS *ar,
  bool sci_enable, bool sci_disable);
 void acpi_pm1_cnt_reset(ACPIREGS *ar);
-- 
2.17.1

[PATCH v4 10/10] qtest/acpi/bios-tables-test: update acpi tables

[isaku . yamahata]

From: Isaku Yamahata 

update golden master acpi tables and empty
bios-tables-test-allowed-diff.h.

Signed-off-by: Isaku Yamahata 
---
 tests/data/acpi/pc/DSDT.nohpet  | Bin 0 -> 4923 bytes
 tests/data/acpi/pc/FACP.nosmm   | Bin 0 -> 116 bytes
 tests/data/acpi/q35/DSDT| Bin 7801 -> 7872 bytes
 tests/data/acpi/q35/DSDT.acpihmat   | Bin 9126 -> 9197 bytes
 tests/data/acpi/q35/DSDT.bridge | Bin 7819 -> 7890 bytes
 tests/data/acpi/q35/DSDT.cphp   | Bin 8265 -> 8336 bytes
 tests/data/acpi/q35/DSDT.dimmpxm| Bin 9455 -> 9526 bytes
 tests/data/acpi/q35/DSDT.ipmibt | Bin 7876 -> 7947 bytes
 tests/data/acpi/q35/DSDT.memhp  | Bin 9160 -> 9231 bytes
 tests/data/acpi/q35/DSDT.mmio64 | Bin 8932 -> 9003 bytes
 tests/data/acpi/q35/DSDT.nohpet | Bin 0 -> 7730 bytes
 tests/data/acpi/q35/DSDT.numamem| Bin 7807 -> 7878 bytes
 tests/data/acpi/q35/DSDT.tis| Bin 8407 -> 8478 bytes
 tests/data/acpi/q35/FACP.nosmm  | Bin 0 -> 244 bytes
 tests/qtest/bios-tables-test-allowed-diff.h |  14 --
 15 files changed, 14 deletions(-)

diff --git a/tests/data/acpi/pc/DSDT.nohpet b/tests/data/acpi/pc/DSDT.nohpet
index 
e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..302deb595c79da7eb8da87baad16a21326e02430
 100644
GIT binary patch
literal 4923
zcmb7IQEwZ^5uW88<>QjHj?~$5tRyBByJ^xQCMjEX5TFpbM@qCr>dd2(7AJtD$e>gZ
zzy~J+q6$H222d0~6%bVPp%09qyjnk@{R#aE`3)%$>#6L_9hagh9*_{w@n+`RZ)SF9
zXO}WL)G+{{cly>#>NYrCGrwAA(U8iVXHoARA9>CAy_w%y
zW|j{@!SO`E7$WUD$b{pX@ZGj6?k9uh%?0vDS6Ni|4J3$sWviTWSbjsSgK@_ag-0Rh
z9g+qEw?jsEpX~w-cituSDd?4DsseqTQ^go2uUpj`xXDnr;X?C8K629JUH30d9)cbB
z;AwLQxB{)de%wM(Lv)sfnU_V@9TltWPBxWz*wO@63RMj8Qa
z_v`%~u0wP%fTK2md(cFGM{Ndf`>4@`z2<4dd3O7*LeWy$Qb9AT`Z%rOBSViYJ(e_M
zuJjmZwI4a)-42}c$SIE+%caMn8bLrcow&-Q-_1b8E0%qDPC6dOXQ>}O>0r(sdOsz#
z)7!X$X<{+@dn1OMg0`B5b7d#4^SSS?LPQB;m2v!w;@>_Pv$TESl!Bn}j6~fm+3+>J
z^RB0vr9D6hQT62cx?Q!|^Nt^dOYTWO^q!yaj%E0Zy=TU}XFTs2jBng~#+dFsglInJlT8N`t%EMn6Nh1hfO8U`6`1mIAbyFu~<}I7dhiG=h<$U#b2g$fg;
zip@l|{4v9ZkfukGfg(iN#D0?`q$2FXMhf9(u9h-l^-`szxj;%>fTeA(RbfDWknx9feS5{wO^RY
z<2G}#i!rZzZp8j}++ZgyR5jezq)RX1NhK~yfBs}ri_6mbp&!BaIzTr+xYO*|f+y9q
z?r!UdVx-oIQGNfQfBzE9s|;Me7FXO$6N5$^x7VUuNkR3mB}BLbEn0HSt>t8td1z8g(9b572%Q?8^UXu5Y24-b!R>){
zt({VJ;|?h2YKbVPro!!rj1>8KVR-s-61^~bSSy_W^C%7>y#3*4Ko7jXoyX2&
zOhY}3DWrOZ=JVJ{j^o)2@ZWFe0em~1Yl8uuYxD_$XOWjIZ7}%P|9s28e}4YEi!r{A
zV1vQnuM+NSp-bO%F%&9JfqyZKTNNvv!zu6^AnC_CpMO(}`gwVs@5i$r^T
zs`?Z8Itfn2lrDOi3tqxYX5hW-y=`=FFV#MuZ{a!t(sd(4KTO>;N$wI;FA6-ng
z)BB+Ki75I3JT8>CuvoH$#tZa##Cli{Q?J$w?zVU9DnIeoDvzxI(}>#Xw0~Pi;e9(Z
z6mWGyU?5w@gj4S_@rA#p%6gXCC4}d#f`1Er3H5Fp_m@_jC&+2V^E_mS_ozhZz2B
zK@Y24F^G_YKI=>%RI*-?&?`aEdy*9|_dcr@2$if635^6n?@LxLp*#>O
zS#cTt0HQ(AWyuueD8p=8x1R1bto)`Wy6
zf}kHt)}(|c1EG=?e=z#7`=dr4>bazlJ%N|UJHWeCF^wwy(5&9|RSW
z^@fDr2!u-3Hzf3pAgC!>Z%XLRK{RDWPu$LM7{468csUv?y8Mme98Y
zp_28Mgx;D4HP#l)k1MrMKX%Z;bM}yW-L5C-C}219g%kX_?0A|S+PHP39WN3nU)Xh$aT7>;%9AhbDDe;)LY2U$ceN#pKHW0oy2@
AJpcdz

literal 0
HcmV?d1

diff --git a/tests/data/acpi/pc/FACP.nosmm b/tests/data/acpi/pc/FACP.nosmm
index 
e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..3cb7d75ea796e0579177c863f16e2aefdc8bed65
 100644
GIT binary patch
literal 116
zcmZ>BbPgzCU|?W0aq@Te2v%^42yk`-iZKGkKx_~V1B?uuFeU>78-
bs5lb?3k#6>pZ`BUM1+CC2*_C4z`y_i8aE69

literal 0
HcmV?d1

diff --git a/tests/data/acpi/q35/DSDT b/tests/data/acpi/q35/DSDT
index 
d25cd7072932886d6967f4023faac1e1fa6e836c..abff765d8e3941a408e6bc703c0b61ec46feccae
 100644
GIT binary patch
delta 101
zcmexqbHJ9%CDiG?fOqnlh=T!I{(IpRG$U3dfh0t}oD3>i3r
soI>3M<+)_q#TgiQ7#Sd71CRn?5D=(j5aEt@4hrU20#dtKiZNam0McOk(iG?dSCo;y%0sxek35@^%

diff --git a/tests/data/acpi/q35/DSDT.acpihmat 
b/tests/data/acpi/q35/DSDT.acpihmat
index 
722e06af83abcde203a2b96a8ec81fd3bab9fc98..5a1d554ee31e64731b42e483e945e5999b79d45a
 100644
GIT binary patch
delta 101
zcmZ4H{??t#CDn~$xCA*mbHsaiy6^`01sFIR7&34K
sIfc3j%5%xIi!(6rFfu^E1|S8(ARti5Ai^E*92Cs41f+Je6l0GP03F2^od5s;

delta 29
lcmaFszRaD=CDvPGsy+0sxbG37r4{

diff --git a/tests/data/acpi/q35/DSDT.bridge b/tests/data/acpi/q35/DSDT.bridge
index 
06bac139d668ddfc7914e258b471a303c9dbd192..184f07c664496c37ffe1475a86358b3c0d702d68
 100644
GIT binary patch
delta 101
zcmeCSy=2Sf66_LkNsfVmasEWER3<0>iG?fOqnlh=T!I{(IpRG$U3dfh0t}oD3>i3r
soI>3M<+)_q#TgiQ7#Sd71CRn?5D=(j5aEt@4hrU20#dtKim^}@0O;@)?*IS*

delta 29
kcmca)+ilC`66_MvEyuvX*guggmC2iHV^0EVOp?*IS*

diff --git a/tests/data/acpi/q35/DSDT.cphp b/tests/data/acpi/q35/DSDT.cphp
index 
2b933ac482e6883efccbd7d6c96089602f2c0b4d..8e164c4a1cc46f552c834601c356d345287c9750
 100644
GIT binary patch
delta 101
zcmX@iG?fOqnlh=T!I{(IpRG$U3dfh0t}oD3>i3r
soI>3M<+)_q#TgiQ7#Sd71CRn?5D=(j5aEt@4hrU20#dtKicw!40Kp9vJpcdz

delta 29
kcmbQ>c+!E(CDk(iG?dSCo*1D1^|JV{n=q6Vdmmo)Hj(87G7v2EB00UM7ZOfgMvAhfYff5Vq7H)0DAcp?*IS*

delta 29
lcmeCSJ7UY_66_LkM2>-hQEMVsDw8+Y#KM)E6B$>@0sw=v2=4#@

diff --git a/tests/data/acpi/q35/DSDT.memhp b/tests/data/acpi/q35/DSDT.memhp
index 

[PATCH v4 06/10] acpi: add test case for smm unsupported -machine smm=off

[isaku . yamahata]

From: Isaku Yamahata 

Reviewed-by: Igor Mammedov 
Signed-off-by: Isaku Yamahata 
---
 tests/qtest/bios-tables-test.c | 76 ++
 1 file changed, 76 insertions(+)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 669202fc95..592c074ec7 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -957,6 +957,39 @@ static void test_acpi_piix4_tcg_memhp(void)
 free_test_data();
 }
 
+static void test_acpi_piix4_tcg_nosmm(void)
+{
+test_data data;
+
+memset(, 0, sizeof(data));
+data.machine = MACHINE_PC;
+data.variant = ".nosmm";
+test_acpi_one("-machine smm=off", );
+free_test_data();
+}
+
+static void test_acpi_piix4_tcg_smm_compat(void)
+{
+test_data data;
+
+memset(, 0, sizeof(data));
+data.machine = MACHINE_PC;
+data.variant = ".smm-compat";
+test_acpi_one("-global PIIX4_PM.smm-compat=on", );
+free_test_data();
+}
+
+static void test_acpi_piix4_tcg_smm_compat_nosmm(void)
+{
+test_data data;
+
+memset(, 0, sizeof(data));
+data.machine = MACHINE_PC;
+data.variant = ".smm-compat-nosmm";
+test_acpi_one("-global PIIX4_PM.smm-compat=on -machine smm=off", );
+free_test_data();
+}
+
 static void test_acpi_q35_tcg_numamem(void)
 {
 test_data data;
@@ -969,6 +1002,39 @@ static void test_acpi_q35_tcg_numamem(void)
 free_test_data();
 }
 
+static void test_acpi_q35_tcg_nosmm(void)
+{
+test_data data;
+
+memset(, 0, sizeof(data));
+data.machine = MACHINE_Q35;
+data.variant = ".nosmm";
+test_acpi_one("-machine smm=off", );
+free_test_data();
+}
+
+static void test_acpi_q35_tcg_smm_compat(void)
+{
+test_data data;
+
+memset(, 0, sizeof(data));
+data.machine = MACHINE_Q35;
+data.variant = ".smm-compat";
+test_acpi_one("-global ICH9-LPC.smm-compat=on", );
+free_test_data();
+}
+
+static void test_acpi_q35_tcg_smm_compat_nosmm(void)
+{
+test_data data;
+
+memset(, 0, sizeof(data));
+data.machine = MACHINE_Q35;
+data.variant = ".smm-compat-nosmm";
+test_acpi_one("-global ICH9-LPC.smm-compat=on -machine smm=off", );
+free_test_data();
+}
+
 static void test_acpi_piix4_tcg_numamem(void)
 {
 test_data data;
@@ -1325,6 +1391,16 @@ int main(int argc, char *argv[])
 qtest_add_func("acpi/q35/memhp", test_acpi_q35_tcg_memhp);
 qtest_add_func("acpi/piix4/numamem", test_acpi_piix4_tcg_numamem);
 qtest_add_func("acpi/q35/numamem", test_acpi_q35_tcg_numamem);
+qtest_add_func("acpi/piix4/nosmm", test_acpi_piix4_tcg_nosmm);
+qtest_add_func("acpi/piix4/smm-compat",
+   test_acpi_piix4_tcg_smm_compat);
+qtest_add_func("acpi/piix4/smm-compat-nosmm",
+   test_acpi_piix4_tcg_smm_compat_nosmm);
+qtest_add_func("acpi/q35/nosmm", test_acpi_q35_tcg_nosmm);
+qtest_add_func("acpi/q35/smm-compat",
+   test_acpi_q35_tcg_smm_compat);
+qtest_add_func("acpi/q35/smm-compat-nosmm",
+   test_acpi_q35_tcg_smm_compat_nosmm);
 qtest_add_func("acpi/piix4/dimmpxm", test_acpi_piix4_tcg_dimm_pxm);
 qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm);
 qtest_add_func("acpi/piix4/acpihmat", test_acpi_piix4_tcg_acpi_hmat);
-- 
2.17.1

[PATCH v4 09/10] acpi: add test case for -no-hpet

[isaku . yamahata]

From: Isaku Yamahata 

Reviewed-by: Igor Mammedov 
Signed-off-by: Isaku Yamahata 
---
 tests/qtest/bios-tables-test.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 592c074ec7..3fb7ed0c46 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -990,6 +990,17 @@ static void test_acpi_piix4_tcg_smm_compat_nosmm(void)
 free_test_data();
 }
 
+static void test_acpi_piix4_tcg_nohpet(void)
+{
+test_data data;
+
+memset(, 0, sizeof(data));
+data.machine = MACHINE_PC;
+data.variant = ".nohpet";
+test_acpi_one("-no-hpet", );
+free_test_data();
+}
+
 static void test_acpi_q35_tcg_numamem(void)
 {
 test_data data;
@@ -1035,6 +1046,17 @@ static void test_acpi_q35_tcg_smm_compat_nosmm(void)
 free_test_data();
 }
 
+static void test_acpi_q35_tcg_nohpet(void)
+{
+test_data data;
+
+memset(, 0, sizeof(data));
+data.machine = MACHINE_Q35;
+data.variant = ".nohpet";
+test_acpi_one("-no-hpet", );
+free_test_data();
+}
+
 static void test_acpi_piix4_tcg_numamem(void)
 {
 test_data data;
@@ -1396,11 +1418,13 @@ int main(int argc, char *argv[])
test_acpi_piix4_tcg_smm_compat);
 qtest_add_func("acpi/piix4/smm-compat-nosmm",
test_acpi_piix4_tcg_smm_compat_nosmm);
+qtest_add_func("acpi/piix4/nohpet", test_acpi_piix4_tcg_nohpet);
 qtest_add_func("acpi/q35/nosmm", test_acpi_q35_tcg_nosmm);
 qtest_add_func("acpi/q35/smm-compat",
test_acpi_q35_tcg_smm_compat);
 qtest_add_func("acpi/q35/smm-compat-nosmm",
test_acpi_q35_tcg_smm_compat_nosmm);
+qtest_add_func("acpi/q35/nohpet", test_acpi_q35_tcg_nohpet);
 qtest_add_func("acpi/piix4/dimmpxm", test_acpi_piix4_tcg_dimm_pxm);
 qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm);
 qtest_add_func("acpi/piix4/acpihmat", test_acpi_piix4_tcg_acpi_hmat);
-- 
2.17.1

[PATCH v4 02/10] qtest: update tests/qtest/bios-tables-test-allowed-diff.h

[isaku . yamahata]

From: Isaku Yamahata 

The following tests will modify acpi tables.
prepare qtests to allow acpi table change.
add new tables for new tests.
- tests/data/acpi/pc/DSDT.nohpet
- tests/data/acpi/pc/FACP.nosmm
- tests/data/acpi/q35/DSDT.nohpet
- tests/data/acpi/q35/FACP.nosmm

Acked-by: Igor Mammedov 
Signed-off-by: Isaku Yamahata 
---
 tests/data/acpi/pc/DSDT.nohpet  |  0
 tests/data/acpi/pc/FACP.nosmm   |  0
 tests/data/acpi/q35/DSDT.nohpet |  0
 tests/data/acpi/q35/FACP.nosmm  |  0
 tests/qtest/bios-tables-test-allowed-diff.h | 14 ++
 5 files changed, 14 insertions(+)
 create mode 100644 tests/data/acpi/pc/DSDT.nohpet
 create mode 100644 tests/data/acpi/pc/FACP.nosmm
 create mode 100644 tests/data/acpi/q35/DSDT.nohpet
 create mode 100644 tests/data/acpi/q35/FACP.nosmm

diff --git a/tests/data/acpi/pc/DSDT.nohpet b/tests/data/acpi/pc/DSDT.nohpet
new file mode 100644
index 00..e69de29bb2
diff --git a/tests/data/acpi/pc/FACP.nosmm b/tests/data/acpi/pc/FACP.nosmm
new file mode 100644
index 00..e69de29bb2
diff --git a/tests/data/acpi/q35/DSDT.nohpet b/tests/data/acpi/q35/DSDT.nohpet
new file mode 100644
index 00..e69de29bb2
diff --git a/tests/data/acpi/q35/FACP.nosmm b/tests/data/acpi/q35/FACP.nosmm
new file mode 100644
index 00..e69de29bb2
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h 
b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..95592459c5 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,15 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/pc/FACP.nosmm",
+"tests/data/acpi/pc/DSDT.nohpet",
+"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.numamem",
+"tests/data/acpi/q35/FACP.nosmm",
+"tests/data/acpi/q35/DSDT.nohpet",
+"tests/data/acpi/q35/DSDT.dimmpxm",
+"tests/data/acpi/q35/DSDT.acpihmat",
-- 
2.17.1

[PATCH v4 00/10] ACPI related fixes to comform the spec better

[isaku . yamahata]

From: Isaku Yamahata 

Miscellaneous bug fixes related to ACPI to play nice with guest BIOSes/OSes
by conforming to ACPI spec better.

Changes from v3:
- rename compat property name to smm-compat and add it to ICH9-LPC and PIIX4_PM
- MMCFG memory region to use dword memory region when possible
- fix max base address of MMCFG memory region
- add more test cases with smm-compat=on and/or piix4

Changes from v2:
- improved commit message
- introduced compat property x-smm-compat-5
- _CRS for MMCONFIG resource, read MMCONFIG info from qemu, generate resource
  instead of dynamically reading chipset configuration.

Changes from v1:
- fixed style issue with fixes to checkpatch.pl
- fixed make check breakage
- added ACPI table tests
- update comment message to include acpi table diff

Isaku Yamahata (9):
  checkpatch: don't emit warning on newly created acpi data files
  qtest: update tests/qtest/bios-tables-test-allowed-diff.h
  ich9, piix4: add properoty, smm-compat, to keep compatibility of SMM
  acpi/core: always set SCI_EN when SMM isn't supported
  acpi: set fadt.smi_cmd to zero when SMM is not supported
  acpi: add test case for smm unsupported -machine smm=off
  hw/i386: declare ACPI mother board resource for MMCONFIG region
  acpi: add test case for -no-hpet
  qtest/acpi/bios-tables-test: update acpi tables

Sean Christopherson (1):
  i386: acpi: Don't build HPET ACPI entry if HPET is disabled

 hw/acpi/core.c|  11 +++-
 hw/acpi/ich9.c|   2 +-
 hw/acpi/piix4.c   |   5 +-
 hw/core/machine.c |   5 +-
 hw/i386/acpi-build.c  |  81 ++--
 hw/isa/lpc_ich9.c |   1 +
 hw/isa/vt82c686.c |   2 +-
 include/hw/acpi/acpi.h|   4 +-
 include/hw/acpi/ich9.h|   1 +
 scripts/checkpatch.pl |   4 +-
 tests/data/acpi/pc/DSDT.nohpet| Bin 0 -> 4923 bytes
 tests/data/acpi/pc/FACP.nosmm | Bin 0 -> 116 bytes
 tests/data/acpi/q35/DSDT  | Bin 7801 -> 7872 bytes
 tests/data/acpi/q35/DSDT.acpihmat | Bin 9126 -> 9197 bytes
 tests/data/acpi/q35/DSDT.bridge   | Bin 7819 -> 7890 bytes
 tests/data/acpi/q35/DSDT.cphp | Bin 8265 -> 8336 bytes
 tests/data/acpi/q35/DSDT.dimmpxm  | Bin 9455 -> 9526 bytes
 tests/data/acpi/q35/DSDT.ipmibt   | Bin 7876 -> 7947 bytes
 tests/data/acpi/q35/DSDT.memhp| Bin 9160 -> 9231 bytes
 tests/data/acpi/q35/DSDT.mmio64   | Bin 8932 -> 9003 bytes
 tests/data/acpi/q35/DSDT.nohpet   | Bin 0 -> 7730 bytes
 tests/data/acpi/q35/DSDT.numamem  | Bin 7807 -> 7878 bytes
 tests/data/acpi/q35/DSDT.tis  | Bin 8407 -> 8478 bytes
 tests/data/acpi/q35/FACP.nosmm| Bin 0 -> 244 bytes
 tests/qtest/bios-tables-test.c| 100 ++
 25 files changed, 203 insertions(+), 13 deletions(-)
 create mode 100644 tests/data/acpi/pc/DSDT.nohpet
 create mode 100644 tests/data/acpi/pc/FACP.nosmm
 create mode 100644 tests/data/acpi/q35/DSDT.nohpet
 create mode 100644 tests/data/acpi/q35/FACP.nosmm

-- 
2.17.1

[PATCH v4 08/10] i386: acpi: Don't build HPET ACPI entry if HPET is disabled

[isaku . yamahata]

From: Sean Christopherson 

Omit HPET AML if the HPET is disabled, QEMU is not emulating it and the
guest may get confused by seeing HPET in the ACPI tables without a
"physical" device present.

The change of DSDT when -no-hpet is as follows.

@@ -141,47 +141,6 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS "
 }
 }

-Scope (_SB)
-{
-Device (HPET)
-{
-Name (_HID, EisaId ("PNP0103") /* HPET System Timer */)  // _HID: 
Hardware ID
-Name (_UID, Zero)  // _UID: Unique ID
-OperationRegion (HPTM, SystemMemory, 0xFED0, 0x0400)
-Field (HPTM, DWordAcc, Lock, Preserve)
-{
-VEND,   32,
-PRD,32
-}
-
-Method (_STA, 0, NotSerialized)  // _STA: Status
-{
-Local0 = VEND /* \_SB_.HPET.VEND */
-Local1 = PRD /* \_SB_.HPET.PRD_ */
-Local0 >>= 0x10
-If (((Local0 == Zero) || (Local0 == 0x)))
-{
-Return (Zero)
-}
-
-If (((Local1 == Zero) || (Local1 > 0x05F5E100)))
-{
-Return (Zero)
-}
-
-Return (0x0F)
-}
-
-Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
-{
-Memory32Fixed (ReadOnly,
-0xFED0, // Address Base
-0x0400, // Address Length
-)
-})
-}
-}
-
 Scope (_SB.PCI0)
 {
 Device (ISA)

Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Igor Mammedov 
Signed-off-by: Sean Christopherson 
---
 hw/i386/acpi-build.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 30326f69b3..aaff9a406d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1290,7 +1290,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
 aml_append(sb_scope, dev);
 aml_append(dsdt, sb_scope);
 
-build_hpet_aml(dsdt);
+if (misc->has_hpet) {
+build_hpet_aml(dsdt);
+}
 build_piix4_isa_bridge(dsdt);
 build_isa_devices_aml(dsdt);
 if (pm->pcihp_bridge_en || pm->pcihp_root_en) {
@@ -1337,7 +1339,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
 
 aml_append(dsdt, sb_scope);
 
-build_hpet_aml(dsdt);
+if (misc->has_hpet) {
+build_hpet_aml(dsdt);
+}
 build_q35_isa_bridge(dsdt);
 build_isa_devices_aml(dsdt);
 build_q35_pci0_int(dsdt);
-- 
2.17.1

[PATCH v4 07/10] hw/i386: declare ACPI mother board resource for MMCONFIG region

[isaku . yamahata]

From: Isaku Yamahata 

Declare PNP0C01 device to reserve MMCONFIG region to conform to the
spec better and play nice with guest BIOSes/OSes.

According to PCI Firmware Specification[0], MMCONFIG region must be
reserved by declaring a motherboard resource. It's optional to reserve
the region in memory map by Int 15 E820h or EFIGetMemoryMap.
Guest Linux checks if the MMCFG region is reserved by bios memory map
or ACPI resource. If it's not reserved, Linux falls back to legacy PCI
configuration access.

TDVF [1] [2] doesn't reserve MMCONFIG the region in memory map.
On the other hand OVMF reserves it in memory map without declaring a
motherboard resource. With memory map reservation, linux guest uses
MMCONFIG region. However it doesn't comply to PCI Firmware
specification.

[0] PCI Firmware specification Revision 3.2
  4.1.2 MCFG Table Description table 4-2 NOTE 2
  If the operating system does not natively comprehend reserving the
  MMCFG region, The MMCFG region must e reserved by firmware. ...
  For most systems, the mortheroard resource would appear at the root
  of the ACPI namespace (under \_SB)...
  The resource can optionally be returned in Int15 E820h or
  EFIGetMemoryMap as reserved memory but must always be reported
  through ACPI as a motherboard resource

[1] TDX: Intel Trust Domain Extension

https://software.intel.com/content/www/us/en/develop/articles/intel-trust-domain-extensions.html
[2] TDX Virtual Firmware
https://github.com/tianocore/edk2-staging/tree/TDVF

The change to DSDT is as follows.

@@ -68,32 +68,51 @@

 If ((CDW3 != Local0))
 {
 CDW1 |= 0x10
 }

 CDW3 = Local0
 }
 Else
 {
 CDW1 |= 0x04
 }

 Return (Arg3)
 }
 }
+
+Device (DRAC)
+{
+Name (_HID, "PNP0C01" /* System Board */)  // _HID: Hardware ID
+Name (RBUF, ResourceTemplate ()
+{
+DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, 
NonCacheable, ReadWrite,
+0x, // Granularity
+0xB000, // Range Minimum
+0xB000, // Range Maximum
+0x, // Translation Offset
+0x1000, // Length
+,, , AddressRangeMemory, TypeStatic)
+})
+Method (_CRS, 0, Serialized)  // _CRS: Current Resource Settings
+{
+Return (RBUF) /* \_SB_.DRAC.RBUF */
+}
+}
 }

 Scope (_SB)
 {
 Device (HPET)
 {
 Name (_HID, EisaId ("PNP0103") /* HPET System Timer */)  // _HID: 
Hardware ID
 Name (_UID, Zero)  // _UID: Unique ID
 OperationRegion (HPTM, SystemMemory, 0xFED0, 0x0400)
 Field (HPTM, DWordAcc, Lock, Preserve)
 {
 VEND,   32,
 PRD,32
 }

 Method (_STA, 0, NotSerialized)  // _STA: Status

Signed-off-by: Isaku Yamahata 
---
 hw/i386/acpi-build.c | 55 +++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e3386ae674..30326f69b3 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1072,6 +1072,55 @@ static void build_q35_pci0_int(Aml *table)
 aml_append(table, sb_scope);
 }
 
+static Aml *build_q35_dram_controller(AcpiMcfgInfo *mcfg)
+{
+Aml *dev;
+Aml *rbuf;
+Aml *resource_template;
+Aml *rbuf_name;
+Aml *crs;
+
+/* DRAM controller */
+dev = aml_device("DRAC");
+aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C01")));
+
+resource_template = aml_resource_template();
+if (mcfg->base + mcfg->size - 1 >= (1ULL << 32)) {
+aml_append(resource_template,
+   aml_qword_memory(AML_POS_DECODE,
+AML_MIN_FIXED,
+AML_MAX_FIXED,
+AML_NON_CACHEABLE,
+AML_READ_WRITE,
+0x,
+mcfg->base,
+mcfg->base,
+0x,
+mcfg->size));
+} else {
+aml_append(resource_template,
+   aml_dword_memory(AML_POS_DECODE,
+AML_MIN_FIXED,
+AML_MAX_FIXED,
+AML_NON_CACHEABLE,
+AML_READ_WRITE,
+0x,
+mcfg->base,
+

[PATCH v4 05/10] acpi: set fadt.smi_cmd to zero when SMM is not supported

[isaku . yamahata]

From: Isaku Yamahata 

>From table 5.9 SMI_CMD of ACPI spec
> This field is reserved and must be zero on system
> that does not support System Management mode.

When smm is not enabled, set it to zero to comform to the spec.
When -machine smm=off is passed, the change to FACP is as follows.

@@ -1,46 +1,46 @@
 /*
  * Intel ACPI Component Architecture
  * AML/ASL+ Disassembler version 20180105 (64-bit version)
  * Copyright (c) 2000 - 2018 Intel Corporation
  *
- * Disassembly of tests/data/acpi/q35/FACP, Fri Feb  5 16:57:04 2021
+ * Disassembly of /tmp/aml-1OQYX0, Fri Feb  5 16:57:04 2021
  *
  * ACPI Data Table [FACP]
  *
  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
  */

 [000h    4]Signature : "FACP"[Fixed ACPI 
Description Table (FADT)]
 [004h 0004   4] Table Length : 00F4
 [008h 0008   1] Revision : 03
-[009h 0009   1] Checksum : 1F
+[009h 0009   1] Checksum : D6
 [00Ah 0010   6]   Oem ID : "BOCHS "
 [010h 0016   8] Oem Table ID : "BXPCFACP"
 [018h 0024   4] Oem Revision : 0001
 [01Ch 0028   4]  Asl Compiler ID : "BXPC"
 [020h 0032   4]Asl Compiler Revision : 0001

 [024h 0036   4] FACS Address : 
 [028h 0040   4] DSDT Address : 
 [02Ch 0044   1]Model : 01
 [02Dh 0045   1]   PM Profile : 00 [Unspecified]
 [02Eh 0046   2]SCI Interrupt : 0009
-[030h 0048   4] SMI Command Port : 00B2
-[034h 0052   1]ACPI Enable Value : 02
-[035h 0053   1]   ACPI Disable Value : 03
+[030h 0048   4] SMI Command Port : 
+[034h 0052   1]ACPI Enable Value : 00
+[035h 0053   1]   ACPI Disable Value : 00
 [036h 0054   1]   S4BIOS Command : 00
 [037h 0055   1]  P-State Control : 00
 [038h 0056   4] PM1A Event Block Address : 0600
 [03Ch 0060   4] PM1B Event Block Address : 
 [040h 0064   4]   PM1A Control Block Address : 0604
 [044h 0068   4]   PM1B Control Block Address : 
 [048h 0072   4]PM2 Control Block Address : 
 [04Ch 0076   4]   PM Timer Block Address : 0608
 [050h 0080   4]   GPE0 Block Address : 0620
 [054h 0084   4]   GPE1 Block Address : 
 [058h 0088   1]   PM1 Event Block Length : 04
 [059h 0089   1] PM1 Control Block Length : 02
 [05Ah 0090   1] PM2 Control Block Length : 00
 [05Bh 0091   1]PM Timer Block Length : 04
 [05Ch 0092   1]GPE0 Block Length : 10
 [05Dh 0093   1]GPE1 Block Length : 00

Reviewed-by: Igor Mammedov 
Signed-off-by: Isaku Yamahata 
---
 hw/i386/acpi-build.c | 18 +++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f56d699c7f..e3386ae674 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -139,6 +139,14 @@ const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio = {
 static void init_common_fadt_data(MachineState *ms, Object *o,
   AcpiFadtData *data)
 {
+X86MachineState *x86ms = X86_MACHINE(ms);
+/*
+ * "ICH9-LPC" or "PIIX4_PM" has "smm-compat" property to keep the old
+ * behavior for compatibility irrelevant to smm_enabled, which doesn't
+ * comforms to ACPI spec.
+ */
+bool smm_enabled = object_property_get_bool(o, "smm-compat", NULL) ?
+true : x86_machine_is_smm_enabled(x86ms);
 uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
 AmlAddressSpace as = AML_AS_SYSTEM_IO;
 AcpiFadtData fadt = {
@@ -159,12 +167,16 @@ static void init_common_fadt_data(MachineState *ms, 
Object *o,
 .rtc_century = RTC_CENTURY,
 .plvl2_lat = 0xfff /* C2 state not supported */,
 .plvl3_lat = 0xfff /* C3 state not supported */,
-.smi_cmd = ACPI_PORT_SMI_CMD,
+.smi_cmd = smm_enabled ? ACPI_PORT_SMI_CMD : 0,
 .sci_int = object_property_get_uint(o, ACPI_PM_PROP_SCI_INT, NULL),
 .acpi_enable_cmd =
-object_property_get_uint(o, ACPI_PM_PROP_ACPI_ENABLE_CMD, NULL),
+smm_enabled ?
+object_property_get_uint(o, ACPI_PM_PROP_ACPI_ENABLE_CMD, NULL) :
+0,
 .acpi_disable_cmd =
-object_property_get_uint(o, ACPI_PM_PROP_ACPI_DISABLE_CMD, NULL),
+smm_enabled ?
+object_property_get_uint(o, ACPI_PM_PROP_ACPI_DISABLE_CMD, NULL) :
+0,
 .pm1a_evt = { .space_id = as, .bit_width = 4 * 8, .address = io },
 .pm1a_cnt = { .space_id = as, .bit_width = 2 * 8,
   .address = io + 0x04 },
-- 
2.17.1

[PATCH v4 01/10] checkpatch: don't emit warning on newly created acpi data files

[isaku . yamahata]

From: Isaku Yamahata 

Newly created acpi data files(tests/data/acpi/) cause false positive
warning.
If file names are acpi expected file, don't emit warning.

Fixes: e625ba2a41 ("checkpatch: fix acpi check with multiple file name")
Signed-off-by: Isaku Yamahata 
---
 scripts/checkpatch.pl | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e47ad878d8..40c9cc7def 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1530,7 +1530,9 @@ sub process {
($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ ||
 $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
 ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ 
&&
- (defined($1) || defined($2) {
+ (defined($1) || defined($2 &&
+  !(($realfile ne '') &&
+($realfile eq $acpi_testexpected))) {
$reported_maintainer_file = 1;
WARN("added, moved or deleted file(s), does MAINTAINERS 
need updating?\n" . $herecurr);
}
-- 
2.17.1

Re: [PATCH 0/4] hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409

[Bin Meng]

Hi Alex,

On Tue, Feb 16, 2021 at 12:48 AM Alexander Bulekov  wrote:
>
> Hi Bin,
> Thank you for this. I ran through the OSS-Fuzz tests again, and it found
> one thing:

Thanks for testing. Are there instructions to run OSS-Fuzz tests myself?

> Maybe this is already much better than the current state of the code, so
> this one can be fixed in a later patch?

Depend on when Philippe can pick up this sereis, but I can also try to
have a quick look :)

>
> cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest \
> -m 512M -nodefaults -device sdhci-pci,sd-spec-version=3 \
> -device sd-card,drive=mydrive \
> -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
> -nographic -qtest stdio
> outl 0xcf8 0x80001010
> outl 0xcfc 0xe000
> outl 0xcf8 0x80001001
> outl 0xcfc 0x0600
> write 0xe02c 0x1 0x05
> write 0xe005 0x1 0x02
> write 0xe007 0x1 0x01
> write 0xe028 0x1 0x10
> write 0x0 0x1 0x23
> write 0x2 0x1 0x08
> write 0xe00c 0x1 0x01
> write 0xe00e 0x1 0x20
> write 0xe00f 0x1 0x00
> write 0xe00c 0x1 0x32
> write 0xe004 0x2 0x0200
> write 0xe028 0x1 0x00
> write 0xe003 0x1 0x40
> EOF
>
>
> ==1730971==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x61531880 at pc 0x55d070f2c6d9 bp 0x7ffdcb63f130 sp 0x7ffdcb63f128
> READ of size 4 at 0x61531880 thread T0
> #0 0x55d070f2c6d8 in ldl_he_p bswap.h:347:5
> #1 0x55d070f2c6d8 in ldn_he_p bswap.h:546:1
> #2 0x55d070f2c6d8 in flatview_write_continue 
> build/../softmmu/physmem.c:2775:19
> #3 0x55d070f219eb in flatview_write build/../softmmu/physmem.c:2816:14
> #4 0x55d070f219eb in address_space_write build/../softmmu/physmem.c:2908:18
> #5 0x55d07040de4a in dma_memory_rw_relaxed include/sysemu/dma.h:88:12
> #6 0x55d07040de4a in dma_memory_rw include/sysemu/dma.h:127:12
> #7 0x55d07040de4a in dma_memory_write include/sysemu/dma.h:163:12
> #8 0x55d07040de4a in sdhci_sdma_transfer_multi_blocks 
> build/../hw/sd/sdhci.c:619:13
> #9 0x55d07041d15b in sdhci_write build/../hw/sd/sdhci.c:1134:21
> #10 0x55d07123b1ac in memory_region_write_accessor 
> build/../softmmu/memory.c:491:5
> #11 0x55d07123acab in access_with_adjusted_size 
> build/../softmmu/memory.c:552:18
> #12 0x55d07123a4b0 in memory_region_dispatch_write build/../softmmu/memory.c
> #13 0x55d070f2c29b in flatview_write_continue 
> build/../softmmu/physmem.c:2776:23
> #14 0x55d070f219eb in flatview_write build/../softmmu/physmem.c:2816:14
> #15 0x55d070f219eb in address_space_write build/../softmmu/physmem.c:2908:18

Regards,
Bin

Re: [PATCH] hw/display/tcx: Drop unnecessary code for handling BGR format outputs

[Richard Henderson]

On 2/15/21 2:21 AM, Peter Maydell wrote:
> For a long time now the UI layer has guaranteed that the console
> surface is always 32 bits per pixel, RGB. The TCX code already
> assumes 32bpp, but it still has some checks of is_surface_bgr()
> in an attempt to support 32bpp BGR. is_surface_bgr() will always
> return false for the qemu_console_surface(), unless the display
> device itself has deliberately created an alternate-format
> surface via a function like qemu_create_displaysurface_from().
> 
> Drop the never-used BGR-handling code, and assert that we have
> a 32-bit surface rather than just doing nothing if it isn't.
> 
> Signed-off-by: Peter Maydell 
> ---
> Tested by booting Linux on an SS-5.
> ---
>  hw/display/tcx.c | 31 ---
>  1 file changed, 8 insertions(+), 23 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH 0/9] arm: drop dead code for non-32-bit-RGB surfaces

[Richard Henderson]

On 2/15/21 2:32 AM, Peter Maydell wrote:
> Peter Maydell (9):
>   hw/arm/musicpal: Remove dead code for non-32-bit-RGB surfaces
>   hw/display/tc6393xb: Remove dead code for handling non-32bpp surfaces
>   hw/display/tc6393xb: Expand out macros in template header
>   hw/display/tc6393xb: Inline tc6393xb_draw_graphic32() at its callsite
>   hw/display/omap_lcdc: Expand out macros in template header
>   hw/display/omap_lcdc: Drop broken bigendian ifdef
>   hw/display/omap_lcdc: Fix coding style issues in template header
>   hw/display/omap_lcdc: Inline template header into C file
>   hw/display/omap_lcdc: Delete unnecessary macro

Reviewed-by: Richard Henderson 

r~