Re: [PATCH v7 4/4] s390: do not call memory_region_allocate_system_memory() multiple times

[Christian Borntraeger]

On 28.09.19 03:28, Peter Xu wrote:
> On Fri, Sep 27, 2019 at 03:33:20PM +0200, Igor Mammedov wrote:
>> On Thu, 26 Sep 2019 07:52:35 +0800
>> Peter Xu  wrote:
>>
>>> On Wed, Sep 25, 2019 at 01:51:05PM +0200, Igor Mammedov wrote:
 On Wed, 25 Sep 2019 11:27:00 +0800
 Peter Xu  wrote:
   
> On Tue, Sep 24, 2019 at 10:47:51AM -0400, Igor Mammedov wrote:  
>> s390 was trying to solve limited KVM memslot size issue by abusing
>> memory_region_allocate_system_memory(), which breaks API contract
>> where the function might be called only once.
>>
>> Beside an invalid use of API, the approach also introduced migration
>> issue, since RAM chunks for each KVM_SLOT_MAX_BYTES are transferred in
>> migration stream as separate RAMBlocks.
>>
>> After discussion [1], it was agreed to break migration from older
>> QEMU for guest with RAM >8Tb (as it was relatively new (since 2.12)
>> and considered to be not actually used downstream).
>> Migration should keep working for guests with less than 8TB and for
>> more than 8TB with QEMU 4.2 and newer binary.
>> In case user tries to migrate more than 8TB guest, between incompatible
>> QEMU versions, migration should fail gracefully due to non-exiting
>> RAMBlock ID or RAMBlock size mismatch.
>>
>> Taking in account above and that now KVM code is able to split too
>> big MemorySection into several memslots, partially revert commit
>>  (bb223055b s390-ccw-virtio: allow for systems larger that 7.999TB)
>> and use kvm_set_max_memslot_size() to set KVMSlot size to
>> KVM_SLOT_MAX_BYTES.
>>
>> 1) [PATCH RFC v2 4/4] s390: do not call  
>> memory_region_allocate_system_memory() multiple times
>>
>> Signed-off-by: Igor Mammedov 
>
> Acked-by: Peter Xu 
>
> IMHO it would be good to at least mention bb223055b9 in the commit
> message even if not with a "Fixed:" tag.  May be amended during commit
> if anyone prefers.  

 /me confused, bb223055b9 is mentioned in commit message  
>>>
>>> I'm sorry, I overlooked that.
>>>

> Also, this only applies the split limitation to s390.  Would that be a
> good thing to some other archs as well?  

 Don't we have the similar bitmap size issue in KVM for other archs?  
>>>
>>> Yes I thought we had.  So I feel like it would be good to also allow
>>> other archs to support >8TB mem as well.  Thanks,
>> Another question, Is there another archs with that much RAM that are
>> available/used in real life (if not I'd wait for demand to arise first)?
> 
> I don't know, so it was a pure question besides the series.  Sorry if
> that holds your series somehow, it was not my intention.
> 
>>
>> If we are to generalize it to other targets, then instead of using
>> arbitrary memslot max size per target, we could just hardcode or get
>> from KVM, max supported size of bitmap and use that to calculate
>> kvm_max_slot_size depending on target page size.
> 
> Right, I think if so hard code would be fine for now, and probably can
> with a smallest one across all archs (should depend on the smallest
> page size, I guess).
> 
>>
>> Then there wouldn't be need for having machine specific code
>> to care about it and pick/set arbitrary values.
>>
>> Another aspect to think about if we are to enable it for
>> other targets is memslot accounting. It doesn't affect s390
>> but other targets that support memory hotplug now assume 1:1
>> relation between memoryregion:memslot, which currently holds
>> true but would need to amended in case split is enabled there.
> 
> I didn't know this.  So maybe it makes more sense to have s390 only
> here.  Thanks,

OK. So shall I take the series as is via the s390 tree?
I would like to add the following patch on top if nobody minds:

Subject: [PATCH 1/1] s390/kvm: split kvm mem slots at 4TB

Instead of splitting at an unaligned address, we can simply split at
4TB.

Signed-off-by: Christian Borntraeger 
---
 target/s390x/kvm.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index ad2dd14f7e78..611f56f4b5ac 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -126,12 +126,11 @@
 /*
  * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
  * as the dirty bitmap must be managed by bitops that take an int as
- * position indicator. If we have a guest beyond that we will split off
- * new subregions. The split must happen on a segment boundary (1MB).
+ * position indicator. This would end at an unaligned  address
+ * (0x7f0). As future variants might provide larger pages
+ * and to make all addresses properly aligned, let us split at 4TB.
  */
-#define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
-#define SEG_MSK (~0xfULL)
-#define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & 
SEG_MSK)
+#define KVM_SLOT_MAX_BYTES 4096UL*1024*1024*1024
 
 static CPUWatchpoi

Re: [PATCH v2 24/33] spapr, xics, xive: Move set_irq from SpaprIrq to SpaprInterruptController

[Greg Kurz]

On Mon, 30 Sep 2019 12:41:39 +1000
David Gibson  wrote:

> On Fri, Sep 27, 2019 at 04:27:12PM +0200, Greg Kurz wrote:
> > On Fri, 27 Sep 2019 15:50:19 +1000
> > David Gibson  wrote:
> > 
> > > This method depends only on the active irq controller.  Now that we've
> > > formalized the notion of active controller we can dispatch directly 
> > > through
> > > that, rather than dispatching via SpaprIrq with the dual version having
> > > to do a second conditional dispatch.
> > > 
> > > Signed-off-by: David Gibson 
> > > ---
> > >  hw/intc/spapr_xive.c   | 12 +++
> > >  hw/intc/xics_spapr.c   |  9 +
> > >  hw/ppc/spapr_irq.c | 41 ++
> > >  include/hw/ppc/spapr_irq.h |  4 +++-
> > >  4 files changed, 34 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > > index ff1a175b44..52d5e71793 100644
> > > --- a/hw/intc/spapr_xive.c
> > > +++ b/hw/intc/spapr_xive.c
> > > @@ -553,6 +553,17 @@ static int 
> > > spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> > >  return 0;
> > >  }
> > >  
> > > +static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, 
> > > int val)
> > > +{
> > > +SpaprXive *xive = SPAPR_XIVE(intc);
> > > +
> > > +if (kvm_irqchip_in_kernel()) {
> > > +kvmppc_xive_source_set_irq(&xive->source, irq, val);
> > > +} else {
> > > +xive_source_set_irq(&xive->source, irq, val);
> > > +}
> > > +}
> > > +
> > >  static void spapr_xive_class_init(ObjectClass *klass, void *data)
> > >  {
> > >  DeviceClass *dc = DEVICE_CLASS(klass);
> > > @@ -574,6 +585,7 @@ static void spapr_xive_class_init(ObjectClass *klass, 
> > > void *data)
> > >  sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
> > >  sicc->claim_irq = spapr_xive_claim_irq;
> > >  sicc->free_irq = spapr_xive_free_irq;
> > > +sicc->set_irq = spapr_xive_set_irq;
> > >  }
> > >  
> > >  static const TypeInfo spapr_xive_info = {
> > > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > > index 224fe1efcd..02372697f6 100644
> > > --- a/hw/intc/xics_spapr.c
> > > +++ b/hw/intc/xics_spapr.c
> > > @@ -373,6 +373,14 @@ static void 
> > > xics_spapr_free_irq(SpaprInterruptController *intc, int irq)
> > >  memset(&ics->irqs[srcno], 0, sizeof(ICSIRQState));
> > >  }
> > >  
> > > +static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, 
> > > int val)
> > > +{
> > > +ICSState *ics = ICS_SPAPR(intc);
> > > +uint32_t srcno = irq - ics->offset;
> > > +
> > > +ics_set_irq(ics, srcno, val);
> > 
> > And we have:
> > 
> > void ics_set_irq(void *opaque, int srcno, int val)
> > {
> > ICSState *ics = (ICSState *)opaque;
> > 
> > if (kvm_irqchip_in_kernel()) {
> > ics_kvm_set_irq(ics, srcno, val);
> > return;
> > }
> > 
> > if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
> > ics_set_irq_lsi(ics, srcno, val);
> > } else {
> > ics_set_irq_msi(ics, srcno, val);
> > }
> > }
> > 
> > The kvm_irqchip_in_kernel() block would fit better in xics_spapr_set_irq(),
> > like it is already the case for XIVE.
> 
> Hmm.. I don't really see why you say that.
> 

I mean this:

static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val)
{
IcsSpapr *icss = ICS_SPAPR(intc);
ICSState *ics = &icss->parent;
uint32_t srcno = irq - ics->offset;

if (kvm_irqchip_in_kernel()) {
ics_kvm_set_irq(ics, srcno, val);
} else {
ics_set_irq(ics, srcno, val);
}
}

It is very similar to spapr_xive_set_irq() and looks nicer to me.

> > Maybe do it now while here ?
> > 
> > Anyway,
> > 
> > Reviewed-by: Greg Kurz 
> > 
> > > +}
> > > +
> > >  static void ics_spapr_class_init(ObjectClass *klass, void *data)
> > >  {
> > >  DeviceClass *dc = DEVICE_CLASS(klass);
> > > @@ -384,6 +392,7 @@ static void ics_spapr_class_init(ObjectClass *klass, 
> > > void *data)
> > >  sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
> > >  sicc->claim_irq = xics_spapr_claim_irq;
> > >  sicc->free_irq = xics_spapr_free_irq;
> > > +sicc->set_irq = xics_spapr_set_irq;
> > >  }
> > >  
> > >  static const TypeInfo ics_spapr_info = {
> > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > > index dfa875b7cd..4922062908 100644
> > > --- a/hw/ppc/spapr_irq.c
> > > +++ b/hw/ppc/spapr_irq.c
> > > @@ -123,14 +123,6 @@ static int 
> > > spapr_irq_post_load_xics(SpaprMachineState *spapr, int version_id)
> > >  return 0;
> > >  }
> > >  
> > > -static void spapr_irq_set_irq_xics(void *opaque, int irq, int val)
> > > -{
> > > -SpaprMachineState *spapr = opaque;
> > > -uint32_t srcno = irq - spapr->ics->offset;
> > > -
> > > -ics_set_irq(spapr->ics, srcno, val);
> > > -}
> > > -
> > >  static void spapr_irq_reset_xics(SpaprMachineState *spapr, Error **errp)
> > >  {
> > >  Error *local_err = NULL;
> > > @@ -159,7 +151,6 @@ SpaprIrq spapr_irq_xi

Re: [PATCH v2 21/33] spapr, xics, xive: Move cpu_intc_create from SpaprIrq to SpaprInterruptController

[David Gibson]

On Mon, Sep 30, 2019 at 07:28:45AM +0200, Cédric Le Goater wrote:
> On 30/09/2019 03:49, David Gibson wrote:
> > On Fri, Sep 27, 2019 at 12:16:49PM +0200, Greg Kurz wrote:
> >> On Fri, 27 Sep 2019 15:50:16 +1000
> >> David Gibson  wrote:
> >>
> >>> This method essentially represents code which belongs to the interrupt
> >>> controller, but needs to be called on all possible intcs, rather than
> >>> just the currently active one.  The "dual" version therefore calls
> >>> into the xics and xive versions confusingly.
> >>>
> >>> Handle this more directly, by making it instead a method on the intc
> >>> backend, and always calling it on every backend that exists.
> >>>
> >>> While we're there, streamline the error reporting a bit.
> >>>
> >>> Signed-off-by: David Gibson 
> > [snip]
> >>> @@ -525,6 +469,30 @@ static void spapr_irq_check(SpaprMachineState 
> >>> *spapr, Error **errp)
> >>>  /*
> >>>   * sPAPR IRQ frontend routines for devices
> >>>   */
> >>> +int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> >>> +  PowerPCCPU *cpu, Error **errp)
> >>> +{
> >>> +if (spapr->xive) {
> >>> +SpaprInterruptController *intc = SPAPR_INTC(spapr->xive);
> >>> +SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> >>> +
> >>> +if (sicc->cpu_intc_create(intc, cpu, errp) < 0) {
> >>> +return -1;
> >>> +}
> >>> +}
> >>> +
> >>> +if (spapr->ics) {
> >>> +SpaprInterruptController *intc = SPAPR_INTC(spapr->ics);
> >>> +SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> >>> +
> >>> +if (sicc->cpu_intc_create(intc, cpu, errp) < 0) {
> >>> +return -1;
> >>> +}
> >>> +}
> >>> +
> >>
> >> Instead of these hooks, what about open-coding spapr_xive_cpu_intc_create()
> >> and xics_spapr_cpu_intc_create() directly here, like you already did for 
> >> the
> >> ICS and the XIVE objects in spapr_irq_init() ?
> > 
> > I'd prefer not to.  The idea is I want to treat this as basically:
> > 
> > foreach_possible_intc(intc)
> > intc::cpu_intc_create(...)
> > 
> > If I find time I might indeed replace the explicit ics and xive
> > pointers with just an array of SpaprInterruptController *.
> 
> Or you could use object_child_foreach() and check for the type. If we had
> a helper object_child_foreach_type(), we could use it elsewhere.

I thought about that, but I don't think it quite works.  The
complication is that the xics device is made explicitly a child of the
machine, but the xive device has mmio, so it's a SusBusDevice sitting
on the root bus instead.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

[no subject]

[Sergio Lopez]

Hi,

Commit 137b5cb6ab565cb3781d5337591e155932b4230e (hmp: change
hmp_info_cpus to use query-cpus-fast) updated the "info cpus" commit to
make it more lightweight, but also removed the ability to get the
architecture specific status of each vCPU.

This information was really useful to diagnose certain Guest issues,
without the need of using GDB, which is more intrusive and requires
enabling it in advance.

Is there an alternative way of getting something equivalent to what
"info cpus" provided previously (in 2.10)?

Thanks,
Sergio.


signature.asc
Description: PGP signature

Re: [PATCH] configure: Remove s390 (31-bit mode) from the list of supported CPUs

[David Hildenbrand]

On 28.09.19 21:03, Thomas Huth wrote:
> On IBM Z, KVM in the kernel is only implemented for 64-bit mode, and
> with regards to TCG, we also only support 64-bit host CPUs (see the
> check at the beginning of tcg/s390/tcg-target.inc.c), so we should
> remove s390 (without "x", i.e. the old 31-bit mode CPUs) from the
> list of supported CPUs.
> 
> Signed-off-by: Thomas Huth 
> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 397bb476e1..a4488c6705 100755
> --- a/configure
> +++ b/configure
> @@ -728,7 +728,7 @@ ARCH=
>  # Normalise host CPU name and set ARCH.
>  # Note that this case should only have supported host CPUs, not guests.
>  case "$cpu" in
> -  ppc|ppc64|s390|s390x|sparc64|x32|riscv32|riscv64)
> +  ppc|ppc64|s390x|sparc64|x32|riscv32|riscv64)
>  supported_cpu="yes"
>;;
>ppc64le)
> 

Not sure if that ever worked

Reviewed-by: David Hildenbrand 

-- 

Thanks,

David / dhildenb

Arch info lost in "info cpus"

[Sergio Lopez]

Hi,

Commit 137b5cb6ab565cb3781d5337591e155932b4230e (hmp: change
hmp_info_cpus to use query-cpus-fast) updated the "info cpus" commit to
make it more lightweight, but also removed the ability to get the
architecture specific status of each vCPU.

This information was really useful to diagnose certain Guest issues,
without the need of using GDB, which is more intrusive and requires
enabling it in advance.

Is there an alternative way of getting something equivalent to what
"info cpus" provided previously (in 2.10)?

Thanks,
Sergio.


signature.asc
Description: PGP signature

Re: [PATCH v3 14/18] target/s390x: Rely on unwinding in s390_cpu_tlb_fill

[David Hildenbrand]

On 27.09.19 18:16, Richard Henderson wrote:
> On 9/27/19 4:02 AM, David Hildenbrand wrote:
>> On 26.09.19 18:26, Richard Henderson wrote:
>>> We currently set ilen to AUTO, then overwrite that during
>>> unwinding, then overwrite that for the code access case.
>>>
>>> This can be simplified to setting ilen to our arbitrary
>>> value for the (undefined) code access case, then rely on
>>> unwinding to overwrite that with the correct value for
>>> the data access case.
>>>
>>> Signed-off-by: Richard Henderson 
>>> ---
>>>  target/s390x/excp_helper.c | 23 +++
>>>  1 file changed, 7 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
>>> index 98a1ee8317..8ce992e639 100644
>>> --- a/target/s390x/excp_helper.c
>>> +++ b/target/s390x/excp_helper.c
>>> @@ -96,7 +96,7 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int 
>>> size,
>>>  {
>>>  S390CPU *cpu = S390_CPU(cs);
>>>  
>>> -trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_AUTO);
>>> +trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_UNWIND);
>>
>> Hmm, we always trigger a pgm exceptions, meaning we set
>> cs->exception_index even if we have probe = true. Confused by that.
> 
> This is the CONFIG_USER_ONLY version, for which probe is always false.  
> Perhaps
> I shouldn't have made the function interface identical, but it did appear to
> make things cleaner for most targets.
> 
>>> +trigger_pgm_exception(env, excp, 2);
>>
>> I wonder if it is still worth setting this only conditionally. Most
>> probably not.
> 
> I don't see that it would be.  I hope the comment is clear about this 
> arbitrary
> value is overwritten during unwinding.

It's confusing, but I get it :)

-- 

Thanks,

David / dhildenb

Re: [PATCH v3 02/18] target/s390x: Add ilen to unwind data

[David Hildenbrand]

On 27.09.19 18:02, Richard Henderson wrote:
> On 9/27/19 3:30 AM, David Hildenbrand wrote:
>>> +/* Update ILEN, except for breakpoint, where we didn't load an insn.  
>>> */
>>> +if (ilen) {
>>> +env->int_pgm_ilen = ilen;
>>> +}
>>
>> I am not completely sure about breakpoint handling and which
>> implications we'll have when not setting int_pgm_ilen ...
> 
> Yeah.  Possibly to make it simple I should simply assign 2 as the length of a
> breakpoint, bearing in mind that there is no a s390 exception to be delivered
> -- this is purely a qemu-internal thing, raising EXCP_DEBUG to get back to the
> gdbstub interface.
> 
>> I wonder if that change can help to better handle exceptions during
>> EXECUTE, whereby we have to indicate the EXECUTE instruction and the
>> ilen of the EXECUTE instruction (so the pc and ilen of the original
>> EXECUTE function, not of the EXECUTE target).
> 
> Yes, that's already there.  The ilen of the execute insn is placed in the low 
> 4
> bits of env->ex_value, and that's what we record as ilen within 
> extract_insn().

Ah, good to know. I'll have to review PGM injection, which ILEN we
currently indicate and which one we are supposed to indicate.

> 
>> I don't completely like the current interrupt handling when we have
>> "env->ex_value" in "s390_cpu_exec_interrupt()". I'd love to see that
>> check go, then we can reuse that function easily e.g., in MVC to test
>> and inject exceptions while processing such an interruptible instruction
>> - like MVC.
> 
> I don't think that reusing s390_cpu_exec_interrupt directly is a good idea.
> There's other cleanup that needs to happen when exiting a TB.
> 
> What I think you should do instead is check env_neg(env)->icount_decr, exactly
> like we do at the start of every basic block, and use that as an indication
> that you should exit back to the main loop.

The issue is that when we return to the main loop we really have to
inject an interrupt - otherwise we might simply skip parts of the
(interruptible) instruction and continue with the next one.

However, with I/O interrupts, we can actually race against other VCPUs.
So the I/O interrupt might be gone by the time we arrive in the main loop.

-- 

Thanks,

David / dhildenb

Re: [PATCH] configure: Remove s390 (31-bit mode) from the list of supported CPUs

[Thomas Huth]

On 30/09/2019 09.51, David Hildenbrand wrote:
> On 28.09.19 21:03, Thomas Huth wrote:
>> On IBM Z, KVM in the kernel is only implemented for 64-bit mode, and
>> with regards to TCG, we also only support 64-bit host CPUs (see the
>> check at the beginning of tcg/s390/tcg-target.inc.c), so we should
>> remove s390 (without "x", i.e. the old 31-bit mode CPUs) from the
>> list of supported CPUs.
>>
>> Signed-off-by: Thomas Huth 
>> ---
>>  configure | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/configure b/configure
>> index 397bb476e1..a4488c6705 100755
>> --- a/configure
>> +++ b/configure
>> @@ -728,7 +728,7 @@ ARCH=
>>  # Normalise host CPU name and set ARCH.
>>  # Note that this case should only have supported host CPUs, not guests.
>>  case "$cpu" in
>> -  ppc|ppc64|s390|s390x|sparc64|x32|riscv32|riscv64)
>> +  ppc|ppc64|s390x|sparc64|x32|riscv32|riscv64)
>>  supported_cpu="yes"
>>;;
>>ppc64le)
>>
> 
> Not sure if that ever worked

I think it likely worked with dyngen (the predecessor of TCG), see
commit fb3e5849bb1 ... but I think it's broken since QEMU switched from
dyngen to TCG.

 Thomas

[Bug 1101210] Re: qemu 1.4.2: usb keyboard not fully working

[Roland Christmann]

Same issue with the qemu version 4.1.0
Host: Windows 10
Guest: Archlinux 5.0.10

showkey output :

keycode 100   # Alt Gr
keycode 29# Left Control
keycode 97# Right Contol
keycode 56# Left Alt
no output # '> < \' key, should be 86

If I change the keyboard layout on the Host (Windows 10), showkey
reports different keycode:

Keyboard layout Belgian (Comma) AZERTY
key 'A' keycode 30 (VirtualBox reports keycode 16)

Keyboard layout US QWERTY
key 'A' keycode 16

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1101210

Title:
  qemu 1.4.2: usb keyboard not fully working

Status in QEMU:
  New

Bug description:
  When using the usb keyboard, I can't type the | character. I'm using
  german keyboard layout (de) on the host and inside the guest. As a
  guest OS, I use Linux (e.g. a recent KNOPPIX cd). To obtain the |
  character on a german keyboard, I need to press AltGr + the < or >
  key, i.e. the key right to the left shift.

  The qemu command line is something like this:
  ./qemu-system-i386 -device pci-ohci -device usb-kbd
  I also tried
  ./qemu-system-i386 -usb -usbdevice keyboard
  with the same effect.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1101210/+subscriptions

Re: [PATCH v2 29/33] spapr, xics, xive: Move SpaprIrq::reset hook logic into activate/deactivate

[David Gibson]

On Mon, Sep 30, 2019 at 08:11:56AM +0200, Cédric Le Goater wrote:
> On 27/09/2019 07:50, David Gibson wrote:
> > It turns out that all the logic in the SpaprIrq::reset hooks (and some in
> > the SpaprIrq::post_load hooks) isn't really related to resetting the irq
> > backend (that's handled by the backends' own reset routines).  Rather its
> > about getting the backend ready to be the active interrupt controller or
> > stopping being the active interrupt controller - reset (and post_load) is
> > just the only time that changes at present.
> 
> This is a 'critical' part which impacts all the migration cases: 
> 
> ic-mode=xics,xive,dual + kernel_irqchip=on/off + TCG

Yes... and?

> > To make this flow clearer, move the logic into the explicit backend
> > activate and deactivate hooks.
> 
> I don't see where the hooks are called ?

spapr_irq_reset()
  -> spapr_irq_update_active_intc()
-> set_active_intc()
  -> activate/deactivate hooks

Similarly via spapr_irq_post_load().

I'm hoping to add one at CAS time to avoid the CAS reboot, too.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

[Bug 1101210] Re: qemu 1.4.2: usb keyboard not fully working

[Roland Christmann]

With qemu version 2.9.94

Host: Windows 10
Guest: Archlinux 5.0.10

showkey output :

keycode 56 press   # Alt Gr
keycode 29 release # Alt Gr
keycode 56 release # Alt Gr

keycode 29 press   # Left Control
keycode 29 release # Left Control

keycode 29 press   # Right Contol
keycode 29 release # Right Contol

keycode 56 press   # Left Alt
keycode 56 release # Left Alt

keycode 86 press   # '> < \' key
keycode 86 release # '> < \' key

As you can see the key 'Alt Gr' is not correctly mapped, it should
return 100

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1101210

Title:
  qemu 1.4.2: usb keyboard not fully working

Status in QEMU:
  New

Bug description:
  When using the usb keyboard, I can't type the | character. I'm using
  german keyboard layout (de) on the host and inside the guest. As a
  guest OS, I use Linux (e.g. a recent KNOPPIX cd). To obtain the |
  character on a german keyboard, I need to press AltGr + the < or >
  key, i.e. the key right to the left shift.

  The qemu command line is something like this:
  ./qemu-system-i386 -device pci-ohci -device usb-kbd
  I also tried
  ./qemu-system-i386 -usb -usbdevice keyboard
  with the same effect.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1101210/+subscriptions

Re: [PATCH v2 24/33] spapr, xics, xive: Move set_irq from SpaprIrq to SpaprInterruptController

[David Gibson]

On Mon, Sep 30, 2019 at 09:22:16AM +0200, Greg Kurz wrote:
> On Mon, 30 Sep 2019 12:41:39 +1000
> David Gibson  wrote:
> 
> > On Fri, Sep 27, 2019 at 04:27:12PM +0200, Greg Kurz wrote:
> > > On Fri, 27 Sep 2019 15:50:19 +1000
> > > David Gibson  wrote:
> > > 
> > > > This method depends only on the active irq controller.  Now that we've
> > > > formalized the notion of active controller we can dispatch directly 
> > > > through
> > > > that, rather than dispatching via SpaprIrq with the dual version having
> > > > to do a second conditional dispatch.
> > > > 
> > > > Signed-off-by: David Gibson 
> > > > ---
> > > >  hw/intc/spapr_xive.c   | 12 +++
> > > >  hw/intc/xics_spapr.c   |  9 +
> > > >  hw/ppc/spapr_irq.c | 41 ++
> > > >  include/hw/ppc/spapr_irq.h |  4 +++-
> > > >  4 files changed, 34 insertions(+), 32 deletions(-)
> > > > 
> > > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > > > index ff1a175b44..52d5e71793 100644
> > > > --- a/hw/intc/spapr_xive.c
> > > > +++ b/hw/intc/spapr_xive.c
> > > > @@ -553,6 +553,17 @@ static int 
> > > > spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> > > >  return 0;
> > > >  }
> > > >  
> > > > +static void spapr_xive_set_irq(SpaprInterruptController *intc, int 
> > > > irq, int val)
> > > > +{
> > > > +SpaprXive *xive = SPAPR_XIVE(intc);
> > > > +
> > > > +if (kvm_irqchip_in_kernel()) {
> > > > +kvmppc_xive_source_set_irq(&xive->source, irq, val);
> > > > +} else {
> > > > +xive_source_set_irq(&xive->source, irq, val);
> > > > +}
> > > > +}
> > > > +
> > > >  static void spapr_xive_class_init(ObjectClass *klass, void *data)
> > > >  {
> > > >  DeviceClass *dc = DEVICE_CLASS(klass);
> > > > @@ -574,6 +585,7 @@ static void spapr_xive_class_init(ObjectClass 
> > > > *klass, void *data)
> > > >  sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
> > > >  sicc->claim_irq = spapr_xive_claim_irq;
> > > >  sicc->free_irq = spapr_xive_free_irq;
> > > > +sicc->set_irq = spapr_xive_set_irq;
> > > >  }
> > > >  
> > > >  static const TypeInfo spapr_xive_info = {
> > > > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > > > index 224fe1efcd..02372697f6 100644
> > > > --- a/hw/intc/xics_spapr.c
> > > > +++ b/hw/intc/xics_spapr.c
> > > > @@ -373,6 +373,14 @@ static void 
> > > > xics_spapr_free_irq(SpaprInterruptController *intc, int irq)
> > > >  memset(&ics->irqs[srcno], 0, sizeof(ICSIRQState));
> > > >  }
> > > >  
> > > > +static void xics_spapr_set_irq(SpaprInterruptController *intc, int 
> > > > irq, int val)
> > > > +{
> > > > +ICSState *ics = ICS_SPAPR(intc);
> > > > +uint32_t srcno = irq - ics->offset;
> > > > +
> > > > +ics_set_irq(ics, srcno, val);
> > > 
> > > And we have:
> > > 
> > > void ics_set_irq(void *opaque, int srcno, int val)
> > > {
> > > ICSState *ics = (ICSState *)opaque;
> > > 
> > > if (kvm_irqchip_in_kernel()) {
> > > ics_kvm_set_irq(ics, srcno, val);
> > > return;
> > > }
> > > 
> > > if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
> > > ics_set_irq_lsi(ics, srcno, val);
> > > } else {
> > > ics_set_irq_msi(ics, srcno, val);
> > > }
> > > }
> > > 
> > > The kvm_irqchip_in_kernel() block would fit better in 
> > > xics_spapr_set_irq(),
> > > like it is already the case for XIVE.
> > 
> > Hmm.. I don't really see why you say that.
> > 
> 
> I mean this:
> 
> static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int 
> val)
> {
> IcsSpapr *icss = ICS_SPAPR(intc);
> ICSState *ics = &icss->parent;
> uint32_t srcno = irq - ics->offset;
> 
> if (kvm_irqchip_in_kernel()) {
> ics_kvm_set_irq(ics, srcno, val);
> } else {
> ics_set_irq(ics, srcno, val);
> }
> }
> 
> It is very similar to spapr_xive_set_irq() and looks nicer to me.

Eh.. I don't really agree.  Seems to me KVM is essentialy an
implementation detail, so belongs in the ICS proper rather than the
papr specific wrapper.

It's not really obvious where it belongs, because the KVM
implementation only works for the PAPR version of the thing, but the
way we call it doesn't explicitly depend on any PAPR specific
information.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: Arch info lost in "info cpus"

[Dr. David Alan Gilbert]

* Sergio Lopez (s...@redhat.com) wrote:
> Hi,
> 
> Commit 137b5cb6ab565cb3781d5337591e155932b4230e (hmp: change
> hmp_info_cpus to use query-cpus-fast) updated the "info cpus" commit to
> make it more lightweight, but also removed the ability to get the
> architecture specific status of each vCPU.
> 
> This information was really useful to diagnose certain Guest issues,
> without the need of using GDB, which is more intrusive and requires
> enabling it in advance.
> 
> Is there an alternative way of getting something equivalent to what
> "info cpus" provided previously (in 2.10)?

Even the qemp equivalent, query-cpus is deprecated.
(Although we do call the underlying query-cpus in 'info numa' as well)

Dave

> Thanks,
> Sergio.


--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[PATCH] linux-user: add strace for dup3

[Andreas Schwab]

Signed-off-by: Andreas Schwab 
---
 linux-user/strace.list | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/linux-user/strace.list b/linux-user/strace.list
index 63a946642d..863283418e 100644
--- a/linux-user/strace.list
+++ b/linux-user/strace.list
@@ -121,6 +121,9 @@
 #ifdef TARGET_NR_dup2
 { TARGET_NR_dup2, "dup2" , NULL, NULL, NULL },
 #endif
+#ifdef TARGET_NR_dup3
+{ TARGET_NR_dup3, "dup3" , NULL, NULL, NULL },
+#endif
 #ifdef TARGET_NR_epoll_create
 { TARGET_NR_epoll_create, "epoll_create" , NULL, NULL, NULL },
 #endif
-- 
2.23.0


-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 06/20] xics: Create sPAPR specific ICS subtype

[David Gibson]

On Fri, Sep 27, 2019 at 06:05:56PM +0200, Greg Kurz wrote:
> On Thu, 26 Sep 2019 10:56:46 +1000
> David Gibson  wrote:
> 
> > On Wed, Sep 25, 2019 at 10:55:35AM +0200, Cédric Le Goater wrote:
> > > On 25/09/2019 10:40, Greg Kurz wrote:
> > > > On Wed, 25 Sep 2019 16:45:20 +1000
> > > > David Gibson  wrote:
> > > > 
> > > >> We create a subtype of TYPE_ICS specifically for sPAPR.  For now all 
> > > >> this
> > > >> does is move the setup of the PAPR specific hcalls and RTAS calls to
> > > >> the realize() function for this, rather than requiring the PAPR code to
> > > >> explicitly call xics_spapr_init().  In future it will have some more
> > > >> function.
> > > >>
> > > >> Signed-off-by: David Gibson 
> > > >> ---
> > > > 
> > > > LGTM, but for extra safety I would also introduce a SpaprIcsState 
> > > > typedef
> > > 
> > > why ? we have ICS_SPAPR() for the checks already.
> > 
> > Eh.. using typedefs when we haven't actually extended a base type
> > isn't common QOM practice.  Yes, it's not as typesafe as it could be,
> > but I'm not really inclined to go to the extra effort here.
> 
> I'll soon need to extend the base type with a nr_servers field,

Uh.. nr_servers doesn't seem like it belongs in the base ICS type.
That really would conflict with the pnv usage where the ICS is
supposed to just represent the ICS, not the xics as a whole.  If you
need nr_servers information here, it seems like pulling it via a
method in XICSFabric would make more sense.

> and while here with an fd field as well in order to get rid of
> the ugly global in xics.c. I'll go to the extra effort :)

That could go in the derived type.  We already kind of conflate ICS
and XICS-as-a-whole for the PAPR subtype, and the KVM xics is PAPR
only anyway.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: Arch info lost in "info cpus"

[Alex Bennée]

Sergio Lopez  writes:

> Hi,
>
> Commit 137b5cb6ab565cb3781d5337591e155932b4230e (hmp: change
> hmp_info_cpus to use query-cpus-fast) updated the "info cpus" commit to
> make it more lightweight, but also removed the ability to get the
> architecture specific status of each vCPU.
>
> This information was really useful to diagnose certain Guest issues,
> without the need of using GDB, which is more intrusive and requires
> enabling it in advance.

You can always enable the gdbserver from the HMP when you need it.

> Is there an alternative way of getting something equivalent to what
> "info cpus" provided previously (in 2.10)?

info registers

should give you a full dump of the CPU state including the PC.

>
> Thanks,
> Sergio.


--
Alex Bennée

Re: [PATCH] configure: Remove s390 (31-bit mode) from the list of supported CPUs

[Alex Bennée]

Thomas Huth  writes:

> On IBM Z, KVM in the kernel is only implemented for 64-bit mode, and
> with regards to TCG, we also only support 64-bit host CPUs (see the
> check at the beginning of tcg/s390/tcg-target.inc.c), so we should
> remove s390 (without "x", i.e. the old 31-bit mode CPUs) from the
> list of supported CPUs.
>
> Signed-off-by: Thomas Huth 

Reviewed-by: Alex Bennée 

> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index 397bb476e1..a4488c6705 100755
> --- a/configure
> +++ b/configure
> @@ -728,7 +728,7 @@ ARCH=
>  # Normalise host CPU name and set ARCH.
>  # Note that this case should only have supported host CPUs, not guests.
>  case "$cpu" in
> -  ppc|ppc64|s390|s390x|sparc64|x32|riscv32|riscv64)
> +  ppc|ppc64|s390x|sparc64|x32|riscv32|riscv64)
>  supported_cpu="yes"
>;;
>ppc64le)


--
Alex Bennée

Re: [RFC PATCH] configure: deprecate 32 bit build hosts

[Daniel P . Berrangé]

On Thu, Sep 26, 2019 at 10:11:05AM -0700, Richard Henderson wrote:
> On 9/26/19 12:50 AM, Peter Maydell wrote:
> > On Thu, 26 Sep 2019 at 00:31, Alex Bennée  wrote:
> >>
> >> The 32 bit hosts are already a second class citizen especially with
> >> support for running 64 bit guests under TCG. We are also limited by
> >> testing as actual working 32 bit machines are getting quite rare in
> >> developers personal menageries. For TCG supporting newer types like
> >> Int128 is a lot harder with 32 bit calling conventions compared to
> >> their larger bit sized cousins. Fundamentally address space is the
> >> most useful thing for the translator to have even for a 32 bit guest a
> >> 32 bit host is quite constrained.
> >>
> >> As far as I'm aware 32 bit KVM users are even less numerous. Even
> >> ILP32 doesn't make much sense given the address space QEMU needs to
> >> manage.
> > 
> > For KVM we should wait until the kernel chooses to drop support,
> > I think.
> 
> Agreed.  I think this discussion should be more about TCG.
> 
> >> @@ -745,19 +744,22 @@ case "$cpu" in
> >>;;
> >>armv*b|armv*l|arm)
> >>  cpu="arm"
> >> -supported_cpu="yes"
> >>;;
> > 
> > I'll leave others to voice opinions about their architectures,
> > but I still have 32-bit arm in my test set for builds, and
> > I'm pretty sure we have users (raspi users, for a start).
> 
> I'd really like to know what raspi users might be using qemu for.  Depending 
> on
> that answer, perhaps it would be sufficient for arm32 tcg to only support
> 32-bit guests.

I asked on the Fedora development lists for feedback on the idea of
dropping QEMU 32-bit host support

  
https://lists.fedoraproject.org/archives/list/de...@lists.fedoraproject.org/thread/TPAVIC6YANGP2NK4RUOP7OCIOIFIOV3A/

The response was rather underwhealming to say the least, with only one
person explicitly expressing a desire for QEMU to keep 32-bit host
support for armv7l.

The main interesting thing I learnt was that even with 64-bit Raspberry
Pi hardware, it can be desirable to run 32-bit Raspberry Pi supporting
distro, supposedly for performance benefits.

> For context, the discussion that Alex and I were having was about int128_t, 
> and
> how to support that directly in tcg (especially to/from helpers), and how that
> might be vastly easier if we didn't have to consider 32-bit hosts.

I know nothing about TCG internals, but Is it viable to implement int128_t
support only in 64-bit host, leaving 32-bit hosts without it ? Or is this
really a blocking item that is holding back 64-bit host features.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v7 4/4] s390: do not call memory_region_allocate_system_memory() multiple times

[Igor Mammedov]

On Mon, 30 Sep 2019 09:09:59 +0200
Christian Borntraeger  wrote:

> On 28.09.19 03:28, Peter Xu wrote:
> > On Fri, Sep 27, 2019 at 03:33:20PM +0200, Igor Mammedov wrote:  
> >> On Thu, 26 Sep 2019 07:52:35 +0800
> >> Peter Xu  wrote:
> >>  
> >>> On Wed, Sep 25, 2019 at 01:51:05PM +0200, Igor Mammedov wrote:  
>  On Wed, 25 Sep 2019 11:27:00 +0800
>  Peter Xu  wrote:
>  
> > On Tue, Sep 24, 2019 at 10:47:51AM -0400, Igor Mammedov wrote:
> >> s390 was trying to solve limited KVM memslot size issue by abusing
> >> memory_region_allocate_system_memory(), which breaks API contract
> >> where the function might be called only once.
> >>
> >> Beside an invalid use of API, the approach also introduced migration
> >> issue, since RAM chunks for each KVM_SLOT_MAX_BYTES are transferred in
> >> migration stream as separate RAMBlocks.
> >>
> >> After discussion [1], it was agreed to break migration from older
> >> QEMU for guest with RAM >8Tb (as it was relatively new (since 2.12)
> >> and considered to be not actually used downstream).
> >> Migration should keep working for guests with less than 8TB and for
> >> more than 8TB with QEMU 4.2 and newer binary.
> >> In case user tries to migrate more than 8TB guest, between incompatible
> >> QEMU versions, migration should fail gracefully due to non-exiting
> >> RAMBlock ID or RAMBlock size mismatch.
> >>
> >> Taking in account above and that now KVM code is able to split too
> >> big MemorySection into several memslots, partially revert commit
> >>  (bb223055b s390-ccw-virtio: allow for systems larger that 7.999TB)
> >> and use kvm_set_max_memslot_size() to set KVMSlot size to
> >> KVM_SLOT_MAX_BYTES.
> >>
> >> 1) [PATCH RFC v2 4/4] s390: do not call  
> >> memory_region_allocate_system_memory() multiple times
> >>
> >> Signed-off-by: Igor Mammedov   
> >
> > Acked-by: Peter Xu 
> >
> > IMHO it would be good to at least mention bb223055b9 in the commit
> > message even if not with a "Fixed:" tag.  May be amended during commit
> > if anyone prefers.
> 
>  /me confused, bb223055b9 is mentioned in commit message
> >>>
> >>> I'm sorry, I overlooked that.
> >>>  
>   
> > Also, this only applies the split limitation to s390.  Would that be a
> > good thing to some other archs as well?
> 
>  Don't we have the similar bitmap size issue in KVM for other archs?
> >>>
> >>> Yes I thought we had.  So I feel like it would be good to also allow
> >>> other archs to support >8TB mem as well.  Thanks,  
> >> Another question, Is there another archs with that much RAM that are
> >> available/used in real life (if not I'd wait for demand to arise first)?  
> > 
> > I don't know, so it was a pure question besides the series.  Sorry if
> > that holds your series somehow, it was not my intention.
> >   
> >>
> >> If we are to generalize it to other targets, then instead of using
> >> arbitrary memslot max size per target, we could just hardcode or get
> >> from KVM, max supported size of bitmap and use that to calculate
> >> kvm_max_slot_size depending on target page size.  
> > 
> > Right, I think if so hard code would be fine for now, and probably can
> > with a smallest one across all archs (should depend on the smallest
> > page size, I guess).
> >   
> >>
> >> Then there wouldn't be need for having machine specific code
> >> to care about it and pick/set arbitrary values.
> >>
> >> Another aspect to think about if we are to enable it for
> >> other targets is memslot accounting. It doesn't affect s390
> >> but other targets that support memory hotplug now assume 1:1
> >> relation between memoryregion:memslot, which currently holds
> >> true but would need to amended in case split is enabled there.  
> > 
> > I didn't know this.  So maybe it makes more sense to have s390 only
> > here.  Thanks,  
> 
> OK. So shall I take the series as is via the s390 tree?
Yes, I'd appreciate it.

> I would like to add the following patch on top if nobody minds:
> 
> Subject: [PATCH 1/1] s390/kvm: split kvm mem slots at 4TB
> 
> Instead of splitting at an unaligned address, we can simply split at
> 4TB.
> 
> Signed-off-by: Christian Borntraeger 

Looks fine to me

Acked-by: Igor Mammedov 

> ---
>  target/s390x/kvm.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index ad2dd14f7e78..611f56f4b5ac 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -126,12 +126,11 @@
>  /*
>   * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
>   * as the dirty bitmap must be managed by bitops that take an int as
> - * position indicator. If we have a guest beyond that we will split off
> - * new subregions. The split must happen on a segment boundary (1MB).
> + * position indicator. This would end at an un

Re: [PATCH v3 1/3] virtio: Add virito_fs linux headers

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (m...@redhat.com) wrote:
> On Tue, Sep 17, 2019 at 05:00:55PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" 
> >
> 
> typo in subject

oops thanks.

>  
> > Pull in the virtio_fs.h linux header and the constant for the virtiofs
> > ID.
> > 
> > Signed-off-by: Stefan Hajnoczi 
> > Signed-off-by: Vivek Goyal 
> > Signed-off-by: Miklos Szeredi 
> 
> This isn't how we add this normally, we normally add
> the header to update-linux and then run it and add
> the result.

update-linux doesn't need changing because it's got:

  "$tmpdir"/include/linux/*virtio*.h \

> In particular if someone reruns update-linux then the
> ID will be gone and build will fail.
> 
> I suggest we either wait a bit until things land in Linux,
> or add these things in a private header for now,
> with an eye towards moving when header lands in Linux.

I'll rerun update-linux to regenerate this patch and repost the set.

Dave

> 
> 
> > ---
> >  include/standard-headers/linux/virtio_fs.h  | 41 +
> >  include/standard-headers/linux/virtio_ids.h |  1 +
> >  2 files changed, 42 insertions(+)
> >  create mode 100644 include/standard-headers/linux/virtio_fs.h
> > 
> > diff --git a/include/standard-headers/linux/virtio_fs.h 
> > b/include/standard-headers/linux/virtio_fs.h
> > new file mode 100644
> > index 00..00bd7a6fa7
> > --- /dev/null
> > +++ b/include/standard-headers/linux/virtio_fs.h
> > @@ -0,0 +1,41 @@
> > +#ifndef _LINUX_VIRTIO_FS_H
> > +#define _LINUX_VIRTIO_FS_H
> > +/* This header is BSD licensed so anyone can use the definitions to 
> > implement
> > + * compatible drivers/servers.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + * 1. Redistributions of source code must retain the above copyright
> > + *notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *notice, this list of conditions and the following disclaimer in the
> > + *documentation and/or other materials provided with the distribution.
> > + * 3. Neither the name of IBM nor the names of its contributors
> > + *may be used to endorse or promote products derived from this software
> > + *without specific prior written permission.
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS 
> > ``AS IS'' AND
> > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR 
> > PURPOSE
> > + * ARE DISCLAIMED.  IN NO EVENT SHALL IBM OR CONTRIBUTORS BE LIABLE
> > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR 
> > CONSEQUENTIAL
> > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, 
> > STRICT
> > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY 
> > WAY
> > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> > + * SUCH DAMAGE. */
> > +#include "standard-headers/linux/types.h"
> > +#include "standard-headers/linux/virtio_ids.h"
> > +#include "standard-headers/linux/virtio_config.h"
> > +#include "standard-headers/linux/virtio_types.h"
> > +
> > +struct virtio_fs_config {
> > +   /* Filesystem name (UTF-8, not NUL-terminated, padded with NULs) */
> > +   uint8_t tag[36];
> > +
> > +   /* Number of request queues */
> > +   uint32_t num_request_queues;
> > +} QEMU_PACKED;
> > +
> > +#endif /* _LINUX_VIRTIO_FS_H */
> > diff --git a/include/standard-headers/linux/virtio_ids.h 
> > b/include/standard-headers/linux/virtio_ids.h
> > index 32b2f94d1f..73fc004807 100644
> > --- a/include/standard-headers/linux/virtio_ids.h
> > +++ b/include/standard-headers/linux/virtio_ids.h
> > @@ -43,6 +43,7 @@
> >  #define VIRTIO_ID_INPUT18 /* virtio input */
> >  #define VIRTIO_ID_VSOCK19 /* virtio vsock transport */
> >  #define VIRTIO_ID_CRYPTO   20 /* virtio crypto */
> > +#define VIRTIO_ID_FS   26 /* virtio filesystem */
> >  #define VIRTIO_ID_PMEM 27 /* virtio pmem */
> >  
> >  #endif /* _LINUX_VIRTIO_IDS_H */
> > -- 
> > 2.21.0
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PULL 00/11] Qcrypto next patches

[Peter Maydell]

On Fri, 27 Sep 2019 at 14:40, Daniel P. Berrangé  wrote:
>
> The following changes since commit d4e536f336d3d26c9fafa2a2549aaa0b014f5b6b:
>
>   Merge remote-tracking branch 'remotes/ericb/tags/pull-nbd-2019-09-24-v2' 
> into staging (2019-09-26 10:13:39 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/berrange/qemu tags/qcrypto-next-pull-request
>
> for you to fetch changes up to befdba9edd20a896f4082adf6dfb613939a24e0c:
>
>   qcrypto-luks: more rigorous header checking (2019-09-26 16:34:02 +0100)
>
> 
> Refactoring of LUKS support to facilitate keyslot updates
>
> No current functional change is expected with this series.
>
> 
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2
for any user-visible changes.

-- PMM

Re: [PATCH v7 4/4] s390: do not call memory_region_allocate_system_memory() multiple times

[Christian Borntraeger]

On 30.09.19 11:33, Igor Mammedov wrote:
> On Mon, 30 Sep 2019 09:09:59 +0200
> Christian Borntraeger  wrote:
> 
>> On 28.09.19 03:28, Peter Xu wrote:
>>> On Fri, Sep 27, 2019 at 03:33:20PM +0200, Igor Mammedov wrote:  
 On Thu, 26 Sep 2019 07:52:35 +0800
 Peter Xu  wrote:
  
> On Wed, Sep 25, 2019 at 01:51:05PM +0200, Igor Mammedov wrote:  
>> On Wed, 25 Sep 2019 11:27:00 +0800
>> Peter Xu  wrote:
>> 
>>> On Tue, Sep 24, 2019 at 10:47:51AM -0400, Igor Mammedov wrote:
 s390 was trying to solve limited KVM memslot size issue by abusing
 memory_region_allocate_system_memory(), which breaks API contract
 where the function might be called only once.

 Beside an invalid use of API, the approach also introduced migration
 issue, since RAM chunks for each KVM_SLOT_MAX_BYTES are transferred in
 migration stream as separate RAMBlocks.

 After discussion [1], it was agreed to break migration from older
 QEMU for guest with RAM >8Tb (as it was relatively new (since 2.12)
 and considered to be not actually used downstream).
 Migration should keep working for guests with less than 8TB and for
 more than 8TB with QEMU 4.2 and newer binary.
 In case user tries to migrate more than 8TB guest, between incompatible
 QEMU versions, migration should fail gracefully due to non-exiting
 RAMBlock ID or RAMBlock size mismatch.

 Taking in account above and that now KVM code is able to split too
 big MemorySection into several memslots, partially revert commit
  (bb223055b s390-ccw-virtio: allow for systems larger that 7.999TB)
 and use kvm_set_max_memslot_size() to set KVMSlot size to
 KVM_SLOT_MAX_BYTES.

 1) [PATCH RFC v2 4/4] s390: do not call  
 memory_region_allocate_system_memory() multiple times

 Signed-off-by: Igor Mammedov   
>>>
>>> Acked-by: Peter Xu 
>>>
>>> IMHO it would be good to at least mention bb223055b9 in the commit
>>> message even if not with a "Fixed:" tag.  May be amended during commit
>>> if anyone prefers.
>>
>> /me confused, bb223055b9 is mentioned in commit message
>
> I'm sorry, I overlooked that.
>  
>>  
>>> Also, this only applies the split limitation to s390.  Would that be a
>>> good thing to some other archs as well?
>>
>> Don't we have the similar bitmap size issue in KVM for other archs?
>
> Yes I thought we had.  So I feel like it would be good to also allow
> other archs to support >8TB mem as well.  Thanks,  
 Another question, Is there another archs with that much RAM that are
 available/used in real life (if not I'd wait for demand to arise first)?  
>>>
>>> I don't know, so it was a pure question besides the series.  Sorry if
>>> that holds your series somehow, it was not my intention.
>>>   

 If we are to generalize it to other targets, then instead of using
 arbitrary memslot max size per target, we could just hardcode or get
 from KVM, max supported size of bitmap and use that to calculate
 kvm_max_slot_size depending on target page size.  
>>>
>>> Right, I think if so hard code would be fine for now, and probably can
>>> with a smallest one across all archs (should depend on the smallest
>>> page size, I guess).
>>>   

 Then there wouldn't be need for having machine specific code
 to care about it and pick/set arbitrary values.

 Another aspect to think about if we are to enable it for
 other targets is memslot accounting. It doesn't affect s390
 but other targets that support memory hotplug now assume 1:1
 relation between memoryregion:memslot, which currently holds
 true but would need to amended in case split is enabled there.  
>>>
>>> I didn't know this.  So maybe it makes more sense to have s390 only
>>> here.  Thanks,  
>>
>> OK. So shall I take the series as is via the s390 tree?
> Yes, I'd appreciate it.


Paolo, ok it I pick the first 3 patches as well? Can you ack?

Re: [PATCH v2 21/33] spapr, xics, xive: Move cpu_intc_create from SpaprIrq to SpaprInterruptController

[Cédric Le Goater]

On 30/09/2019 08:14, David Gibson wrote:
> On Mon, Sep 30, 2019 at 07:28:45AM +0200, Cédric Le Goater wrote:
>> On 30/09/2019 03:49, David Gibson wrote:
>>> On Fri, Sep 27, 2019 at 12:16:49PM +0200, Greg Kurz wrote:
 On Fri, 27 Sep 2019 15:50:16 +1000
 David Gibson  wrote:

> This method essentially represents code which belongs to the interrupt
> controller, but needs to be called on all possible intcs, rather than
> just the currently active one.  The "dual" version therefore calls
> into the xics and xive versions confusingly.
>
> Handle this more directly, by making it instead a method on the intc
> backend, and always calling it on every backend that exists.
>
> While we're there, streamline the error reporting a bit.
>
> Signed-off-by: David Gibson 
>>> [snip]
> @@ -525,6 +469,30 @@ static void spapr_irq_check(SpaprMachineState 
> *spapr, Error **errp)
>  /*
>   * sPAPR IRQ frontend routines for devices
>   */
> +int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> +  PowerPCCPU *cpu, Error **errp)
> +{
> +if (spapr->xive) {
> +SpaprInterruptController *intc = SPAPR_INTC(spapr->xive);
> +SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> +
> +if (sicc->cpu_intc_create(intc, cpu, errp) < 0) {
> +return -1;
> +}
> +}
> +
> +if (spapr->ics) {
> +SpaprInterruptController *intc = SPAPR_INTC(spapr->ics);
> +SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> +
> +if (sicc->cpu_intc_create(intc, cpu, errp) < 0) {
> +return -1;
> +}
> +}
> +

 Instead of these hooks, what about open-coding spapr_xive_cpu_intc_create()
 and xics_spapr_cpu_intc_create() directly here, like you already did for 
 the
 ICS and the XIVE objects in spapr_irq_init() ?
>>>
>>> I'd prefer not to.  The idea is I want to treat this as basically:
>>>
>>> foreach_possible_intc(intc)
>>> intc::cpu_intc_create(...)
>>>
>>> If I find time I might indeed replace the explicit ics and xive
>>> pointers with just an array of SpaprInterruptController *.
>>
>> Or you could use object_child_foreach() and check for the type. If we had
>> a helper object_child_foreach_type(), we could use it elsewhere.
> 
> I thought about that, but I don't think it quite works.  The
> complication is that the xics device is made explicitly a child of the
> machine, but the xive device has mmio, so it's a SusBusDevice sitting
> on the root bus instead.

PnvXscom works fine with Devices and SysBusDevices.

C. 

Re: Arch info lost in "info cpus"

[Sergio Lopez]

Alex Bennée  writes:

> Sergio Lopez  writes:
>
>> Hi,
>>
>> Commit 137b5cb6ab565cb3781d5337591e155932b4230e (hmp: change
>> hmp_info_cpus to use query-cpus-fast) updated the "info cpus" commit to
>> make it more lightweight, but also removed the ability to get the
>> architecture specific status of each vCPU.
>>
>> This information was really useful to diagnose certain Guest issues,
>> without the need of using GDB, which is more intrusive and requires
>> enabling it in advance.
>
> You can always enable the gdbserver from the HMP when you need it.
>
>> Is there an alternative way of getting something equivalent to what
>> "info cpus" provided previously (in 2.10)?
>
> info registers
>
> should give you a full dump of the CPU state including the PC.
>

Both methods are less convenient that what we had before. Perhaps it'd
be reasonable adding a flag to "info cpus" to give users the options of
having the same behavior as before?

Thanks,
Sergio.


signature.asc
Description: PGP signature

Re: [PATCH] configure: Remove s390 (31-bit mode) from the list of supported CPUs

[Christian Borntraeger]

On 28.09.19 21:03, Thomas Huth wrote:
> On IBM Z, KVM in the kernel is only implemented for 64-bit mode, and
> with regards to TCG, we also only support 64-bit host CPUs (see the
> check at the beginning of tcg/s390/tcg-target.inc.c), so we should
> remove s390 (without "x", i.e. the old 31-bit mode CPUs) from the
> list of supported CPUs.
> 
> Signed-off-by: Thomas Huth 
> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 397bb476e1..a4488c6705 100755
> --- a/configure
> +++ b/configure
> @@ -728,7 +728,7 @@ ARCH=
>  # Normalise host CPU name and set ARCH.
>  # Note that this case should only have supported host CPUs, not guests.
>  case "$cpu" in
> -  ppc|ppc64|s390|s390x|sparc64|x32|riscv32|riscv64)
> +  ppc|ppc64|s390x|sparc64|x32|riscv32|riscv64)
>  supported_cpu="yes"
>;;
>ppc64le)
> 

Thanks applied to s390-next.

Thoughts on VM fence infrastructure

[Felipe Franciosi]

Heyall,

We have a use case where a host should self-fence (and all VMs should
die) if it doesn't hear back from a heartbeat within a certain time
period. Lots of ideas were floated around where libvirt could take
care of killing VMs or a separate service could do it. The concern
with those is that various failures could lead to _those_ services
being unavailable and the fencing wouldn't be enforced as it should.

Ultimately, it feels like Qemu should be responsible for this
heartbeat and exit (or execute a custom callback) on timeout.

Does something already exist for this purpose which could be used?
Would a generic Qemu-fencing infrastructure be something of interest?

Cheers,
F.

Re: [PATCH v7 1/4] kvm: extract kvm_log_clear_one_slot

[Christian Borntraeger]

On 24.09.19 16:47, Igor Mammedov wrote:
> From: Paolo Bonzini 
> 
> We may need to clear the dirty bitmap for more than one KVM memslot.
> First do some code movement with no semantic change.
> 
> Signed-off-by: Paolo Bonzini 
> Signed-off-by: Igor Mammedov 
> Reviewed-by: Peter Xu 
> ---
>  accel/kvm/kvm-all.c | 102 
>  1 file changed, 56 insertions(+), 46 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index b09bad0804..e9e6086c09 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -575,55 +575,13 @@ out:
>  #define KVM_CLEAR_LOG_ALIGN  (qemu_real_host_page_size << 
> KVM_CLEAR_LOG_SHIFT)
>  #define KVM_CLEAR_LOG_MASK   (-KVM_CLEAR_LOG_ALIGN)
>  
> -/**
> - * kvm_physical_log_clear - Clear the kernel's dirty bitmap for range
> - *
> - * NOTE: this will be a no-op if we haven't enabled manual dirty log
> - * protection in the host kernel because in that case this operation
> - * will be done within log_sync().
> - *
> - * @kml: the kvm memory listener
> - * @section: the memory range to clear dirty bitmap
> - */
> -static int kvm_physical_log_clear(KVMMemoryListener *kml,
> -  MemoryRegionSection *section)
> +static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start, 
> uint64_t size)

When applying, I will split this to fit within 80 chars. ok?

Re: [PATCH v5 0/9] qcow2-bitmaps: rewrite reopening logic

[Vladimir Sementsov-Ogievskiy]

28.09.2019 2:36, John Snow wrote:
> 
> 
> On 9/27/19 8:23 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Bitmaps reopening is buggy, reopening-rw just not working at all and
>> reopening-ro may lead to producing broken incremental
>> backup if we do temporary snapshot in a meantime.
>>
>> v5:
>> 01: - add Max's r-b
>>  - fix s/QSIMPLE_INIT/QTAILQ_INIT/ in a comment
>> 02: - add Max's and John's a-b
>> 03: - improve test to check bitmap hashes in more safe way
>> 07: - drop wrong statement from commit message
>>  - log events by hand
>> 08: - drop 'the' from comment
>>  - add Corruption in case of existent IN_USE on RO->RW reopen
>> 09: - add Max's a-b and John's r-b
>>
>> v4: Drop complicated solution around reopening logic [Kevin], fix
>>  the existing bug in a simplest way
>>
>> Vladimir Sementsov-Ogievskiy (9):
>>block: switch reopen queue from QSIMPLEQ to QTAILQ
>>block: reverse order for reopen commits
>>iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW
>>block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps
>>block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()
>>block/qcow2-bitmap: do not remove bitmaps on reopen-ro
>>iotests: add test 260 to check bitmap life after snapshot + commit
>>block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
>>qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit
>>
>>   block/qcow2.h|   5 +-
>>   include/block/block.h|   2 +-
>>   include/block/block_int.h|   6 --
>>   include/block/dirty-bitmap.h |   1 -
>>   block.c  |  53 +--
>>   block/dirty-bitmap.c |  12 ---
>>   block/qcow2-bitmap.c | 164 ++-
>>   block/qcow2.c|  17 +++-
>>   tests/qemu-iotests/165   |  57 +++-
>>   tests/qemu-iotests/165.out   |   4 +-
>>   tests/qemu-iotests/260   |  89 +++
>>   tests/qemu-iotests/260.out   |  52 +++
>>   tests/qemu-iotests/group |   1 +
>>   13 files changed, 343 insertions(+), 120 deletions(-)
>>   create mode 100755 tests/qemu-iotests/260
>>   create mode 100644 tests/qemu-iotests/260.out
>>
> 
> Provisionally staged, pending feedback from Kevin.
> 
> Some minor rebase conflicts against the current bitmaps branch:
> 
> 012/17:[] [-C] 'block/qcow2-bitmap: get rid of
> bdrv_has_changed_persistent_bitmaps'
> 014/17:[0004] [FC] 'block/qcow2-bitmap: do not remove bitmaps on reopen-ro'
> 017/17:[] [-C] 'qcow2-bitmap: move bitmap reopen-rw code to
> qcow2_reopen_commit'
> 
> 
> 12: just context against changed _first and _next prototypes.
> 14: the signature of bdrv_release_dirty_bitmap has changed, which incurs
> a change in the refactor.
> 17: bdrv_can_store... has changed to bdrv_co_can_store...
>  Same for bdrv_remove_persistent.
> 
> 
> Thanks, applied to my bitmaps tree:
> 
> https://github.com/jnsnow/qemu/commits/bitmaps
> https://github.com/jnsnow/qemu.git
> 


Thanks a lot!


-- 
Best regards,
Vladimir

Re: [PATCH v7 4/4] s390: do not call memory_region_allocate_system_memory() multiple times

[Paolo Bonzini]

On 30/09/19 12:04, Christian Borntraeger wrote:
> Paolo, ok it I pick the first 3 patches as well? Can you ack?

Yes, please.

Acked-by: Paolo Bonzini 

Paolo

Re: [RFC PATCH] configure: deprecate 32 bit build hosts

[Peter Maydell]

On Mon, 30 Sep 2019 at 11:26, Alex Bennée  wrote:
> So if lots of people still want 32 bit host support for TCG guests we
> can probably come up with something that keeps existing functionality
> ticking over while leaving the newer architectural features to 64 bit
> hosts only.

I don't think we should do that -- QEMU should offer the same
(emulated) functionality regardless of host.

thanks
-- PMM

Re: [PULL 0/9] target-arm queue

[Peter Maydell]

On Fri, 27 Sep 2019 at 15:42, Peter Maydell  wrote:
>
> target-arm queue: nothing major here, but no point
> sitting on them waiting for more stuff to come along.
>
> thanks
> -- PMM
>
> The following changes since commit 1329132d28bf14b9508f7a1f04a2c63422bc3f99:
>
>   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging 
> (2019-09-26 16:14:03 +0100)
>
> are available in the Git repository at:
>
>   https://git.linaro.org/people/pmaydell/qemu-arm.git 
> tags/pull-target-arm-20190927
>
> for you to fetch changes up to e4e34855e658b78ecac50a651cc847662ff02cfd:
>
>   hw/arm/boot: Use the IEC binary prefix definitions (2019-09-27 11:44:39 
> +0100)
>
> 
> target-arm queue:
>  * Fix the CBAR register implementation for Cortex-A53,
>Cortex-A57, Cortex-A72
>  * Fix direct booting of Linux kernels on emulated CPUs
>which have an AArch32 EL3 (incorrect NSACR settings
>meant they could not access the FPU)
>  * semihosting cleanup: do more work at translate time
>and less work at runtime


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2
for any user-visible changes.

-- PMM

Re: [PULL 00/27] QAPI patches for 2019-09-28

[Peter Maydell]

On Sat, 28 Sep 2019 at 19:45, Markus Armbruster  wrote:
>
> The following changes since commit c6f5012ba5fa834cbd5274b1b8369e2c5d2f5933:
>
>   Merge remote-tracking branch 
> 'remotes/stsquad/tags/pull-testing-next-260919-1' into staging (2019-09-27 
> 15:43:41 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2019-09-28
>
> for you to fetch changes up to c615550df306a7b16e75d21f65ee38898c756bac:
>
>   qapi: Improve source file read error handling (2019-09-28 17:17:48 +0200)
>
> 
> QAPI patches for 2019-09-28
>

Just a note that repo.or.cz seems to be down currently;
I'll retry this pullreq later in the week.

thanks
-- PMM

[PATCH v4 0/3] Add virtio-fs

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

Hi,
  This set of patches adds the core of the virtio-fs support to qemu.
It's no longer marked experimental since both the spec and kernel have
been merged upstream.

  A future set of patches will add the optional DAX mapping support.

  The actual qemu change is pretty minimal, since it's really only
a virtio device with some queues.

Some links:
  Mailing list: https://www.redhat.com/mailman/listinfo/virtio-fs
  Dev tree: Including filesystem daemon: https://gitlab.com/virtio-fs/qemu

v4
  Update the linux headers using the update script directly from the
kernel tree
  No longer marked experimental

Dr. David Alan Gilbert (3):
  virtio: Add virtio_fs linux headers
  virtio: add vhost-user-fs base device
  virtio: add vhost-user-fs-pci device

 configure   |  13 +
 hw/virtio/Makefile.objs |   2 +
 hw/virtio/vhost-user-fs-pci.c   |  85 ++
 hw/virtio/vhost-user-fs.c   | 299 
 include/hw/virtio/vhost-user-fs.h   |  45 +++
 include/standard-headers/linux/virtio_fs.h  |  19 ++
 include/standard-headers/linux/virtio_ids.h |   2 +
 7 files changed, 465 insertions(+)
 create mode 100644 hw/virtio/vhost-user-fs-pci.c
 create mode 100644 hw/virtio/vhost-user-fs.c
 create mode 100644 include/hw/virtio/vhost-user-fs.h
 create mode 100644 include/standard-headers/linux/virtio_fs.h

-- 
2.21.0

[PATCH v4 2/3] virtio: add vhost-user-fs base device

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

The virtio-fs virtio device provides shared file system access using
the FUSE protocol carried over virtio.
The actual file server is implemented in an external vhost-user-fs device
backend process.

Signed-off-by: Stefan Hajnoczi 
Signed-off-by: Sebastien Boeuf 
Signed-off-by: Dr. David Alan Gilbert 
---
 configure |  13 ++
 hw/virtio/Makefile.objs   |   1 +
 hw/virtio/vhost-user-fs.c | 299 ++
 include/hw/virtio/vhost-user-fs.h |  45 +
 4 files changed, 358 insertions(+)
 create mode 100644 hw/virtio/vhost-user-fs.c
 create mode 100644 include/hw/virtio/vhost-user-fs.h

diff --git a/configure b/configure
index 542f6aea3f..204cbe351e 100755
--- a/configure
+++ b/configure
@@ -381,6 +381,7 @@ vhost_crypto=""
 vhost_scsi=""
 vhost_vsock=""
 vhost_user=""
+vhost_user_fs=""
 kvm="no"
 hax="no"
 hvf="no"
@@ -1293,6 +1294,10 @@ for opt do
   ;;
   --enable-vhost-vsock) vhost_vsock="yes"
   ;;
+  --disable-vhost-user-fs) vhost_user_fs="no"
+  ;;
+  --enable-vhost-user-fs) vhost_user_fs="yes"
+  ;;
   --disable-opengl) opengl="no"
   ;;
   --enable-opengl) opengl="yes"
@@ -2236,6 +2241,10 @@ test "$vhost_crypto" = "" && vhost_crypto=$vhost_user
 if test "$vhost_crypto" = "yes" && test "$vhost_user" = "no"; then
   error_exit "--enable-vhost-crypto requires --enable-vhost-user"
 fi
+test "$vhost_user_fs" = "" && vhost_user_fs=$vhost_user
+if test "$vhost_user_fs" = "yes" && test "$vhost_user" = "no"; then
+  error_exit "--enable-vhost-user-fs requires --enable-vhost-user"
+fi
 
 # OR the vhost-kernel and vhost-user values for simplicity
 if test "$vhost_net" = ""; then
@@ -6377,6 +6386,7 @@ echo "vhost-crypto support $vhost_crypto"
 echo "vhost-scsi support $vhost_scsi"
 echo "vhost-vsock support $vhost_vsock"
 echo "vhost-user support $vhost_user"
+echo "vhost-user-fs support $vhost_user_fs"
 echo "Trace backends$trace_backends"
 if have_backend "simple"; then
 echo "Trace output file $trace_file-"
@@ -6873,6 +6883,9 @@ fi
 if test "$vhost_user" = "yes" ; then
   echo "CONFIG_VHOST_USER=y" >> $config_host_mak
 fi
+if test "$vhost_user_fs" = "yes" ; then
+  echo "CONFIG_VHOST_USER_FS=y" >> $config_host_mak
+fi
 if test "$blobs" = "yes" ; then
   echo "INSTALL_BLOBS=yes" >> $config_host_mak
 fi
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 964ce78607..47ffbf22c4 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -11,6 +11,7 @@ common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
 common-obj-$(CONFIG_VIRTIO_MMIO) += virtio-mmio.o
 obj-$(CONFIG_VIRTIO_BALLOON) += virtio-balloon.o
 obj-$(CONFIG_VIRTIO_CRYPTO) += virtio-crypto.o
+obj-$(CONFIG_VHOST_USER_FS) += vhost-user-fs.o
 obj-$(call land,$(CONFIG_VIRTIO_CRYPTO),$(CONFIG_VIRTIO_PCI)) += 
virtio-crypto-pci.o
 obj-$(CONFIG_VIRTIO_PMEM) += virtio-pmem.o
 common-obj-$(call land,$(CONFIG_VIRTIO_PMEM),$(CONFIG_VIRTIO_PCI)) += 
virtio-pmem-pci.o
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
new file mode 100644
index 00..f0df7f4746
--- /dev/null
+++ b/hw/virtio/vhost-user-fs.c
@@ -0,0 +1,299 @@
+/*
+ * Vhost-user filesystem virtio device
+ *
+ * Copyright 2018-2019 Red Hat, Inc.
+ *
+ * Authors:
+ *  Stefan Hajnoczi 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include 
+#include "standard-headers/linux/virtio_fs.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
+#include "qemu/error-report.h"
+#include "hw/virtio/vhost-user-fs.h"
+#include "monitor/monitor.h"
+
+static void vuf_get_config(VirtIODevice *vdev, uint8_t *config)
+{
+VHostUserFS *fs = VHOST_USER_FS(vdev);
+struct virtio_fs_config fscfg = {};
+
+memcpy((char *)fscfg.tag, fs->conf.tag,
+   MIN(strlen(fs->conf.tag) + 1, sizeof(fscfg.tag)));
+
+virtio_stl_p(vdev, &fscfg.num_request_queues, fs->conf.num_request_queues);
+
+memcpy(config, &fscfg, sizeof(fscfg));
+}
+
+static void vuf_start(VirtIODevice *vdev)
+{
+VHostUserFS *fs = VHOST_USER_FS(vdev);
+BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+int ret;
+int i;
+
+if (!k->set_guest_notifiers) {
+error_report("binding does not support guest notifiers");
+return;
+}
+
+ret = vhost_dev_enable_notifiers(&fs->vhost_dev, vdev);
+if (ret < 0) {
+error_report("Error enabling host notifiers: %d", -ret);
+return;
+}
+
+ret = k->set_guest_notifiers(qbus->parent, fs->vhost_dev.nvqs, true);
+if (ret < 0) {
+error_report("Error binding guest notifier: %d", -ret);
+goto err_host_notifiers;
+}
+
+fs->vhost_dev.acked_features = vdev->guest_features;
+ret = vhost_dev_start(&fs->

[PATCH v4 3/3] virtio: add vhost-user-fs-pci device

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

Add the PCI version of vhost-user-fs.

Launch QEMU like this:

  qemu -chardev socket,path=/tmp/vhost-fs.sock,id=chr0
   -device vhost-user-fs-pci,tag=myfs,chardev=chr0

Signed-off-by: Stefan Hajnoczi 
Signed-off-by: Sebastien Boeuf 
Signed-off-by: Dr. David Alan Gilbert 
Reviewed-by: Cornelia Huck 
---
 hw/virtio/Makefile.objs   |  1 +
 hw/virtio/vhost-user-fs-pci.c | 85 +++
 2 files changed, 86 insertions(+)
 create mode 100644 hw/virtio/vhost-user-fs-pci.c

diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 47ffbf22c4..e2f70fbb89 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -15,6 +15,7 @@ obj-$(CONFIG_VHOST_USER_FS) += vhost-user-fs.o
 obj-$(call land,$(CONFIG_VIRTIO_CRYPTO),$(CONFIG_VIRTIO_PCI)) += 
virtio-crypto-pci.o
 obj-$(CONFIG_VIRTIO_PMEM) += virtio-pmem.o
 common-obj-$(call land,$(CONFIG_VIRTIO_PMEM),$(CONFIG_VIRTIO_PCI)) += 
virtio-pmem-pci.o
+obj-$(call land,$(CONFIG_VHOST_USER_FS),$(CONFIG_VIRTIO_PCI)) += 
vhost-user-fs-pci.o
 obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock.o
 
 ifeq ($(CONFIG_VIRTIO_PCI),y)
diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c
new file mode 100644
index 00..933a3f265b
--- /dev/null
+++ b/hw/virtio/vhost-user-fs-pci.c
@@ -0,0 +1,85 @@
+/*
+ * Vhost-user filesystem virtio device PCI glue
+ *
+ * Copyright 2018-2019 Red Hat, Inc.
+ *
+ * Authors:
+ *  Dr. David Alan Gilbert 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/vhost-user-fs.h"
+#include "virtio-pci.h"
+
+struct VHostUserFSPCI {
+VirtIOPCIProxy parent_obj;
+VHostUserFS vdev;
+};
+
+typedef struct VHostUserFSPCI VHostUserFSPCI;
+
+#define TYPE_VHOST_USER_FS_PCI "vhost-user-fs-pci-base"
+
+#define VHOST_USER_FS_PCI(obj) \
+OBJECT_CHECK(VHostUserFSPCI, (obj), TYPE_VHOST_USER_FS_PCI)
+
+static Property vhost_user_fs_pci_properties[] = {
+DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
+   DEV_NVECTORS_UNSPECIFIED),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vhost_user_fs_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+VHostUserFSPCI *dev = VHOST_USER_FS_PCI(vpci_dev);
+DeviceState *vdev = DEVICE(&dev->vdev);
+
+if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
+vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 1;
+}
+
+qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
+object_property_set_bool(OBJECT(vdev), true, "realized", errp);
+}
+
+static void vhost_user_fs_pci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+k->realize = vhost_user_fs_pci_realize;
+set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+dc->props = vhost_user_fs_pci_properties;
+pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+pcidev_k->device_id = 0; /* Set by virtio-pci based on virtio id */
+pcidev_k->revision = 0x00;
+pcidev_k->class_id = PCI_CLASS_STORAGE_OTHER;
+}
+
+static void vhost_user_fs_pci_instance_init(Object *obj)
+{
+VHostUserFSPCI *dev = VHOST_USER_FS_PCI(obj);
+
+virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+TYPE_VHOST_USER_FS);
+}
+
+static const VirtioPCIDeviceTypeInfo vhost_user_fs_pci_info = {
+.base_name = TYPE_VHOST_USER_FS_PCI,
+.non_transitional_name = "vhost-user-fs-pci",
+.instance_size = sizeof(VHostUserFSPCI),
+.instance_init = vhost_user_fs_pci_instance_init,
+.class_init= vhost_user_fs_pci_class_init,
+};
+
+static void vhost_user_fs_pci_register(void)
+{
+virtio_pci_types_register(&vhost_user_fs_pci_info);
+}
+
+type_init(vhost_user_fs_pci_register);
-- 
2.21.0

[PATCH v4 1/3] virtio: Add virtio_fs linux headers

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

Pull in the virtio_fs.h linux header and the constant for the virtiofs
ID; by running scripts/update-linux-headers.sh against
Linus's tree 97f9a3c4eee55b0178b518ae7114a6a53372913d.

Signed-off-by: Stefan Hajnoczi 
Signed-off-by: Vivek Goyal 
Signed-off-by: Dr. David Alan Gilbert 
Signed-off-by: Miklos Szeredi 
---
 include/standard-headers/linux/virtio_fs.h  | 19 +++
 include/standard-headers/linux/virtio_ids.h |  2 ++
 2 files changed, 21 insertions(+)
 create mode 100644 include/standard-headers/linux/virtio_fs.h

diff --git a/include/standard-headers/linux/virtio_fs.h 
b/include/standard-headers/linux/virtio_fs.h
new file mode 100644
index 00..9d88817a6b
--- /dev/null
+++ b/include/standard-headers/linux/virtio_fs.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR 
BSD-3-Clause) */
+
+#ifndef _LINUX_VIRTIO_FS_H
+#define _LINUX_VIRTIO_FS_H
+
+#include "standard-headers/linux/types.h"
+#include "standard-headers/linux/virtio_ids.h"
+#include "standard-headers/linux/virtio_config.h"
+#include "standard-headers/linux/virtio_types.h"
+
+struct virtio_fs_config {
+   /* Filesystem name (UTF-8, not NUL-terminated, padded with NULs) */
+   uint8_t tag[36];
+
+   /* Number of request queues */
+   uint32_t num_request_queues;
+} QEMU_PACKED;
+
+#endif /* _LINUX_VIRTIO_FS_H */
diff --git a/include/standard-headers/linux/virtio_ids.h 
b/include/standard-headers/linux/virtio_ids.h
index 32b2f94d1f..585e07b273 100644
--- a/include/standard-headers/linux/virtio_ids.h
+++ b/include/standard-headers/linux/virtio_ids.h
@@ -43,6 +43,8 @@
 #define VIRTIO_ID_INPUT18 /* virtio input */
 #define VIRTIO_ID_VSOCK19 /* virtio vsock transport */
 #define VIRTIO_ID_CRYPTO   20 /* virtio crypto */
+#define VIRTIO_ID_IOMMU23 /* virtio IOMMU */
+#define VIRTIO_ID_FS   26 /* virtio filesystem */
 #define VIRTIO_ID_PMEM 27 /* virtio pmem */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
-- 
2.21.0

Re: [PATCH v7 0/4] s390: stop abusing memory_region_allocate_system_memory()

[Christian Borntraeger]

On 24.09.19 16:47, Igor Mammedov wrote:
> Changelog:
>   since v6:
> - include and rebase on top of
>[PATCH 0/2] kvm: clear dirty bitmaps from all overlapping memslots
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg646200.html
> - minor fixups suggested during v6 review
> - more testing incl. hacked x86
>   since v5:
> - [1/2] fix migration that wasn't starting and make sure that KVM part
>   is able to handle 1:n MemorySection:memslot arrangement
>   since v3:
> - fix compilation issue
> - advance HVA along with GPA in kvm_set_phys_mem()
>   since v2:
> - break migration from old QEMU (since 2.12-4.1) for guest with >8TB RAM
>   and drop migratable aliases patch as was agreed during v2 review
> - drop 4.2 machines patch as it's not prerequisite anymore
>   since v1:
> - include 4.2 machines patch for adding compat RAM layout on top
> - 2/4 add missing in v1 patch for splitting too big MemorySection on
>   several memslots
> - 3/4 amend code path on alias destruction to ensure that RAMBlock is
>   cleaned properly
> - 4/4 add compat machine code to keep old layout (migration-wise) for
>   4.1 and older machines 
> 
> 
> While looking into unifying guest RAM allocation to use hostmem backends
> for initial RAM (especially when -mempath is used) and retiring
> memory_region_allocate_system_memory() API, leaving only single hostmem 
> backend,
> I was inspecting how currently it is used by boards and it turns out several
> boards abuse it by calling the function several times (despite documented 
> contract
> forbiding it).
> 
> s390 is one of such boards where KVM limitation on memslot size got propagated
> to board design and memory_region_allocate_system_memory() was abused to 
> satisfy
> KVM requirement for max RAM chunk where memory region alias would suffice.
> 
> Unfortunately, memory_region_allocate_system_memory() usage created migration
> dependency where guest RAM is transferred in migration stream as several 
> RAMBlocks
> if it's more than KVM_SLOT_MAX_BYTES. During v2 review it was agreed to ignore
> migration breakage (documenting it in release notes) and leaving only KVM fix.
> 
> In order to replace these several RAM chunks with a single memdev and keep it
> working with KVM memslot size limit, the later was modified to deal with 
> memory section split on several KVMSlots and manual RAM splitting in s390
> was replace by single memory_region_allocate_system_memory() call.
> 
> Tested:
>   * s390 with hacked KVM_SLOT_MAX_BYTES = 128Mb
>   - guest reboot cycle in ping-pong migration
>   * x86 with hacke max memslot = 128 and manual_dirty_log_protect enabled
>   - ping-pong migration with workload dirtying RAM around a split area
> 

Thanks, v7 applied to s390-next.

Re: [PATCH v4 2/3] virtio: add vhost-user-fs base device

[Marc-André Lureau]

Hi

On Mon, Sep 30, 2019 at 2:52 PM Dr. David Alan Gilbert (git)
 wrote:
>
> From: "Dr. David Alan Gilbert" 
>
> The virtio-fs virtio device provides shared file system access using
> the FUSE protocol carried over virtio.
> The actual file server is implemented in an external vhost-user-fs device
> backend process.
>
> Signed-off-by: Stefan Hajnoczi 
> Signed-off-by: Sebastien Boeuf 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  configure |  13 ++
>  hw/virtio/Makefile.objs   |   1 +
>  hw/virtio/vhost-user-fs.c | 299 ++
>  include/hw/virtio/vhost-user-fs.h |  45 +
>  4 files changed, 358 insertions(+)
>  create mode 100644 hw/virtio/vhost-user-fs.c
>  create mode 100644 include/hw/virtio/vhost-user-fs.h
>
> diff --git a/configure b/configure
> index 542f6aea3f..204cbe351e 100755
> --- a/configure
> +++ b/configure
> @@ -381,6 +381,7 @@ vhost_crypto=""
>  vhost_scsi=""
>  vhost_vsock=""
>  vhost_user=""
> +vhost_user_fs=""
>  kvm="no"
>  hax="no"
>  hvf="no"
> @@ -1293,6 +1294,10 @@ for opt do
>;;
>--enable-vhost-vsock) vhost_vsock="yes"
>;;
> +  --disable-vhost-user-fs) vhost_user_fs="no"
> +  ;;
> +  --enable-vhost-user-fs) vhost_user_fs="yes"
> +  ;;
>--disable-opengl) opengl="no"
>;;
>--enable-opengl) opengl="yes"
> @@ -2236,6 +2241,10 @@ test "$vhost_crypto" = "" && vhost_crypto=$vhost_user
>  if test "$vhost_crypto" = "yes" && test "$vhost_user" = "no"; then
>error_exit "--enable-vhost-crypto requires --enable-vhost-user"
>  fi
> +test "$vhost_user_fs" = "" && vhost_user_fs=$vhost_user
> +if test "$vhost_user_fs" = "yes" && test "$vhost_user" = "no"; then
> +  error_exit "--enable-vhost-user-fs requires --enable-vhost-user"
> +fi
>
>  # OR the vhost-kernel and vhost-user values for simplicity
>  if test "$vhost_net" = ""; then
> @@ -6377,6 +6386,7 @@ echo "vhost-crypto support $vhost_crypto"
>  echo "vhost-scsi support $vhost_scsi"
>  echo "vhost-vsock support $vhost_vsock"
>  echo "vhost-user support $vhost_user"
> +echo "vhost-user-fs support $vhost_user_fs"
>  echo "Trace backends$trace_backends"
>  if have_backend "simple"; then
>  echo "Trace output file $trace_file-"
> @@ -6873,6 +6883,9 @@ fi
>  if test "$vhost_user" = "yes" ; then
>echo "CONFIG_VHOST_USER=y" >> $config_host_mak
>  fi
> +if test "$vhost_user_fs" = "yes" ; then
> +  echo "CONFIG_VHOST_USER_FS=y" >> $config_host_mak
> +fi
>  if test "$blobs" = "yes" ; then
>echo "INSTALL_BLOBS=yes" >> $config_host_mak
>  fi
> diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> index 964ce78607..47ffbf22c4 100644
> --- a/hw/virtio/Makefile.objs
> +++ b/hw/virtio/Makefile.objs
> @@ -11,6 +11,7 @@ common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>  common-obj-$(CONFIG_VIRTIO_MMIO) += virtio-mmio.o
>  obj-$(CONFIG_VIRTIO_BALLOON) += virtio-balloon.o
>  obj-$(CONFIG_VIRTIO_CRYPTO) += virtio-crypto.o
> +obj-$(CONFIG_VHOST_USER_FS) += vhost-user-fs.o
>  obj-$(call land,$(CONFIG_VIRTIO_CRYPTO),$(CONFIG_VIRTIO_PCI)) += 
> virtio-crypto-pci.o
>  obj-$(CONFIG_VIRTIO_PMEM) += virtio-pmem.o
>  common-obj-$(call land,$(CONFIG_VIRTIO_PMEM),$(CONFIG_VIRTIO_PCI)) += 
> virtio-pmem-pci.o
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> new file mode 100644
> index 00..f0df7f4746
> --- /dev/null
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -0,0 +1,299 @@
> +/*
> + * Vhost-user filesystem virtio device
> + *
> + * Copyright 2018-2019 Red Hat, Inc.
> + *
> + * Authors:
> + *  Stefan Hajnoczi 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include 
> +#include "standard-headers/linux/virtio_fs.h"
> +#include "qapi/error.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
> +#include "qemu/error-report.h"
> +#include "hw/virtio/vhost-user-fs.h"
> +#include "monitor/monitor.h"
> +
> +static void vuf_get_config(VirtIODevice *vdev, uint8_t *config)
> +{
> +VHostUserFS *fs = VHOST_USER_FS(vdev);
> +struct virtio_fs_config fscfg = {};
> +
> +memcpy((char *)fscfg.tag, fs->conf.tag,
> +   MIN(strlen(fs->conf.tag) + 1, sizeof(fscfg.tag)));
> +
> +virtio_stl_p(vdev, &fscfg.num_request_queues, 
> fs->conf.num_request_queues);
> +
> +memcpy(config, &fscfg, sizeof(fscfg));
> +}
> +
> +static void vuf_start(VirtIODevice *vdev)
> +{
> +VHostUserFS *fs = VHOST_USER_FS(vdev);
> +BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> +VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +int ret;
> +int i;
> +
> +if (!k->set_guest_notifiers) {
> +error_report("binding does not support guest notifiers");
> +return;
> +}
> +
> +ret = vhost_dev_enable_notifiers(&fs->vhost_dev, vdev);
> +if (ret < 0) {
> +error_report("E

[Bug 1633508] Re: libvirt cannot hot insert interfaces to qemu

[Thomas Huth]

This looks like a libvirt bug at a first glance. Have you tried to
report it to the libvirt project? (See https://libvirt.org/bugs.html )
... also, can you re-create the bug with the very latest upstream
version of libvirt and qemu, or does it only occur with an (older?)
version of Ubuntu?

** Changed in: qemu
   Status: New => Incomplete

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1633508

Title:
  libvirt cannot hot insert interfaces to qemu

Status in QEMU:
  Incomplete

Bug description:
  When attempting to hot insert an interface using Ubuntu 16.04.1, I get the 
following
  $ virsh attach-interface --domain gluster1 --type direct \
  > --source test0 --model virtio \
  > --mac 2a:b6:b0:dc:c7:c4 --config --live
  error: Failed to attach interface
  error: internal error: unable to execute QEMU command 'getfd': No file 
descriptor supplied via SCM_RIGHTS

  test0 exists:
  $ ip link show test0
  35: test0:  mtu 1500 qdisc pfifo_fast 
state DOWN mode DEFAULT group default qlen 1000
  link/ether aa:8c:65:2e:79:61 brd ff:ff:ff:ff:ff:ff

  Just in case I did it wrong with direct, I did network
  $ virsh net-list
   Name State  Autostart Persistent
  --
   default  active yes   yes
   mgmtnet0 active yes   yes

  $ virsh attach-interface --domain gluster1 --type network \
  > --source default --model virtio \
  > --mac 2a:b6:b0:dc:c7:c4 --config --live
  error: Failed to attach interface
  error: internal error: unable to execute QEMU command 'getfd': No file 
descriptor supplied via SCM_RIGHTS

  
  This seems to be an old bug, but is still present.  Other relevant 
information:
  $ qemu-system-x86_64 --version
  QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.5), Copyright (c) 
2003-2008 Fabrice Bellard
  $ virsh -v
  1.3.1

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1633508/+subscriptions

Re: [PATCH v12 05/11] numa: Extend CLI to provide initiator information for numa nodes

[Igor Mammedov]

On Fri, 20 Sep 2019 15:43:43 +0800
Tao Xu  wrote:

> In ACPI 6.3 chapter 5.2.27 Heterogeneous Memory Attribute Table (HMAT),
> The initiator represents processor which access to memory. And in 5.2.27.3
> Memory Proximity Domain Attributes Structure, the attached initiator is
> defined as where the memory controller responsible for a memory proximity
> domain. With attached initiator information, the topology of heterogeneous
> memory can be described.
> 
> Extend CLI of "-numa node" option to indicate the initiator numa node-id.
> In the linux kernel, the codes in drivers/acpi/hmat/hmat.c parse and report
> the platform's HMAT tables.
> 
> Reviewed-by: Jingqi Liu 
> Suggested-by: Dan Williams 
> Signed-off-by: Tao Xu 
> ---
> 
> Changes in v12:
> - Fix the bug that a memory-only node without initiator setting
>   doesn't report error. (reported by Danmei Wei)
> 
> No changes in v11.
> 
> Changes in v10:
> - Add machine oprion properties "-machine hmat=on|off" for enabling
>   or disabling HMAT in QEMU.
> - Add more description for initiator option.
> - Report error then HMAT is enable and initiator option is missing.
>   Not allow invaild initiator now. (Igor)
> ---
>  hw/core/machine.c | 71 +++
>  hw/core/numa.c| 24 +++
>  include/sysemu/numa.h |  6 
>  qapi/machine.json | 10 +-
>  qemu-options.hx   | 35 ++---
>  5 files changed, 140 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 1689ad3bf8..087baaf571 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -518,6 +518,20 @@ static void machine_set_nvdimm(Object *obj, bool value, 
> Error **errp)
>  ms->nvdimms_state->is_enabled = value;
>  }
>  
> +static bool machine_get_hmat(Object *obj, Error **errp)
> +{
> +MachineState *ms = MACHINE(obj);
> +
> +return ms->numa_state->hmat_enabled;
> +}
> +
> +static void machine_set_hmat(Object *obj, bool value, Error **errp)
> +{
> +MachineState *ms = MACHINE(obj);
> +
> +ms->numa_state->hmat_enabled = value;
> +}
> +
>  static char *machine_get_nvdimm_persistence(Object *obj, Error **errp)
>  {
>  MachineState *ms = MACHINE(obj);
> @@ -645,6 +659,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
> const CpuInstanceProperties *props, Error 
> **errp)
>  {
>  MachineClass *mc = MACHINE_GET_CLASS(machine);
> +NodeInfo *numa_info = machine->numa_state->nodes;
>  bool match = false;
>  int i;
>  
> @@ -714,6 +729,16 @@ void machine_set_cpu_numa_node(MachineState *machine,
>  match = true;
>  slot->props.node_id = props->node_id;
>  slot->props.has_node_id = props->has_node_id;
> +
> +if (numa_info[props->node_id].initiator_valid &&
> +(props->node_id != numa_info[props->node_id].initiator)) {
> +error_setg(errp, "The initiator of CPU NUMA node %" PRId64
> +   " should be itself.", props->node_id);
> +return;
> +}
> +numa_info[props->node_id].initiator_valid = true;
> +numa_info[props->node_id].has_cpu = true;
> +numa_info[props->node_id].initiator = props->node_id;
>  }
>  
>  if (!match) {
> @@ -960,6 +985,13 @@ static void machine_initfn(Object *obj)
>  
>  if (mc->numa_mem_supported) {
>  ms->numa_state = g_new0(NumaState, 1);
> +object_property_add_bool(obj, "hmat",
> + machine_get_hmat, machine_set_hmat,
> + &error_abort);
> +object_property_set_description(obj, "hmat",
> +"Set on/off to enable/disable "
> +"ACPI Heterogeneous Memory Attribute 
> "
> +"Table (HMAT)", NULL);
>  }
>  
>  /* Register notifier when init is done for sysbus sanity checks */
> @@ -1048,6 +1080,40 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
>  return g_string_free(s, false);
>  }
>  
> +static void numa_validate_initiator(NumaState *nstat)
> +{
> +int i;
> +NodeInfo *numa_info = nstat->nodes;
> +
> +for (i = 0; i < nstat->num_nodes; i++) {
> +if (numa_info[i].initiator == MAX_NODES) {
> +error_report("The initiator of NUMA node %d is missing, use "
> + "'-numa node,initiator' option to declare it.", i);
> +goto err;
> +}
> +
> +if (!numa_info[numa_info[i].initiator].present) {
> +error_report("NUMA node %" PRIu16 " is missing, use "
> + "'-numa node' option to declare it first.",
> + numa_info[i].initiator);
> +goto err;
> +}
> +
> +if (numa_info[numa_info[i].initiator].has_cpu) {
> +numa_info[i].initiator_valid = true;
> +} else {
> + 

Re: [RFC PATCH] configure: deprecate 32 bit build hosts

[Peter Maydell]

On Mon, 30 Sep 2019 at 11:26, Alex Bennée  wrote:
> The int128 support is due to the fact we are going to start to see newer
> architectures with things like 128 bit shadow capability registers and
> they will be a pain to shuffle around in 32 bit generated host code as
> well as requiring writing new extra plumbing for TCG's pre/post amble to
> pass them back and forth between C and generated code. These guest
> architectures may not even be full 64 bit guests so it's not quite as
> simple as saying you can't have 64 bit guests on a 32 bit host.

I think that for int128_t in particular, the ideal answer is
that if the compiler developers want to introduce a new
abstraction like that, they should support it on all targets,
not just half of them...

thanks
-- PMM

Re: [edk2-devel] [Qemu-devel] [PATCH 1/2] q35: implement 128K SMRAM at default SMBASE address

[Laszlo Ersek]

Hi Igor,

On 09/24/19 13:19, Igor Mammedov wrote:
> On Mon, 23 Sep 2019 20:35:02 +0200
> "Laszlo Ersek"  wrote:

>> I've got good results. For this (1/2) QEMU patch:
>>
>> Tested-by: Laszlo Ersek 
>>
>> I tested the following scenarios. In every case, I verified the OVMF
>> log, and also the "info mtree" monitor command's result (i.e. whether
>> "smbase-blackhole" / "smbase-window" were disabled or enabled).
>> Mostly, I diffed these text files between the test scenarios (looking
>> for desired / undesired differences). In the Linux guests, I checked
>> / compared the dmesg too (wrt. the UEFI memmap).
>>
>> - unpatched OVMF (regression test), Fedora guest, normal boot and S3
>>
>> - patched OVMF, but feature disabled with "-global
>>   mch.smbase-smram=off" (another regression test), Fedora guest,
>>   normal boot and S3
>>
>> - patched OVMF, feature enabled, Fedora and various Windows guests
>>   (win7, win8, win10 families, client/server), normal boot and S3
>>
>> - a subset of the above guests, with S3 disabled (-global
>>   ICH9-LPC.disable_s3=1), and obviously S3 resume not tested
>>
>> SEV: used 5.2-ish Linux guest, with S3 disabled (no support under SEV
>> for that now):
>>
>> - unpatched OVMF (regression test), normal boot
>>
>> - patched OVMF but feature disabled on the QEMU cmdline (another
>>   regression test), normal boot
>>
>> - patched OVMF, feature enabled, normal boot.
>>
>> I plan to post the OVMF patches tomorrow, for discussion.
>>
>> (It's likely too early to push these QEMU / edk2 patches right now --
>> we don't know yet if this path will take us to the destination. For
>> now, it certainly looks great.)
>
> Laszlo, thanks for trying it out.
> It's nice to hear that approach is somewhat usable.
> Hopefully we won't have to invent 'paused' cpu mode.
>
> Pls CC me on your patches
> (not that I qualify for reviewing,
> but may be I could learn a thing or two from it)

Considering the plan at [1], the two patch sets [2] [3] should cover
step (01); at least as proof of concept.

[1] [edk2-devel] CPU hotplug using SMM with QEMU+OVMF
http://mid.mail-archive.com/20190830164802.1b17ff26@redhat.com

[2] The current thread:
[Qemu-devel] [PATCH 0/2] q35: mch: allow to lock down 128K RAM at default 
SMBASE address
http://mid.mail-archive.com/20190917130708.10281-1-imammedo@redhat.com

[3] [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" 
feature
http://mid.mail-archive.com/20190924113505.27272-1-lersek@redhat.com

(I'll have to figure out what SMI handler to put in place there, but I'd
like to experiment with that once we can cause a new CPU to start
executing code there, in SMM.)

So what's next?

To me it looks like we need to figure out how QEMU can make the OS call
into SMM (in the GPE cpu hotplug handler), passing in parameters and
such. This would be step (03).

Do you agree?

If so, I'll ask Jiewen about such OS->SMM calls separately, because I
seem to remember that there used to be an "SMM communcation table" of
sorts, for flexible OS->SMM calls. However, it appears to be deprecated
lately.

Hmmm Yes, UEFI 2.8 has "Appendix O - UEFI ACPI Data Table", and it
writes (after defining the table format):

The first use of this UEFI ACPI table format is the SMM
Communication ACPI Table. This table describes a special software
SMI that can be used to initiate inter-mode communication in the OS
present environment by non-firmware agents with SMM code.

Note: The use of the SMM Communication ACPI table is deprecated in
  UEFI spec. 2.7. This is due to the lack of a use case for
  inter-mode communication by non-firmware agents with SMM code
  and support for initiating this form of communication in
  common OSes.

The changelog at the front of the UEFI spec also references the
Mantis#1691 spec ticket, "Remove/Deprecate SMM Communication ACPI Table"
(addressed in UEFI 2.6B).

(I think that must have been a security ticket, because, while I
generally have access to Mantis tickets,
 gives me "Access
Denied" :/ )

Thanks,
Laszlo

[PATCH v2] Disallow colons in the parameter of "-accel"

[Thomas Huth]

Everybody who used something like "-machine accel=kvm:tcg" in the past
might be tempted to specify a similar list with the -accel parameter,
too, for example "-accel kvm:tcg". However, this is not how this
options is thought to be used, since each "-accel" should only take care
of one specific accelerator.

In the long run, we really should rework the "-accel" code completely,
so that it does not set "-machine accel=..." anymore internally, but
is completely independent from "-machine". For the short run, let's
make sure that users cannot use "-accel xyz:tcg", so that we avoid
that we have to deal with such cases in the wild later.

Signed-off-by: Thomas Huth 
---
 v2:
 - Reword the message a little bit (as suggested by Paolo)
 - Fix the cdrom-test - it already contained a bad usage of -accel

 tests/cdrom-test.c | 2 +-
 vl.c   | 5 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/cdrom-test.c b/tests/cdrom-test.c
index 05611da648..34e9974634 100644
--- a/tests/cdrom-test.c
+++ b/tests/cdrom-test.c
@@ -120,7 +120,7 @@ static void test_cdboot(gconstpointer data)
 {
 QTestState *qts;
 
-qts = qtest_initf("-accel kvm:tcg -no-shutdown %s%s", (const char *)data,
+qts = qtest_initf("-M accel=kvm:tcg -no-shutdown %s%s", (const char *)data,
   isoimage);
 boot_sector_test(qts);
 qtest_quit(qts);
diff --git a/vl.c b/vl.c
index 630f5c5e9c..002bf4919e 100644
--- a/vl.c
+++ b/vl.c
@@ -3554,6 +3554,11 @@ int main(int argc, char **argv, char **envp)
 g_slist_free(accel_list);
 exit(0);
 }
+if (optarg && strchr(optarg, ':')) {
+error_report("Don't use ':' with -accel, "
+ "use -M accel=... for now instead");
+exit(1);
+}
 opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
 false, &error_abort);
 qemu_opt_set(opts, "accel", optarg, &error_abort);
-- 
2.18.1

Re: [edk2-devel] [Qemu-devel] [PATCH 1/2] q35: implement 128K SMRAM at default SMBASE address

[Igor Mammedov]

On Mon, 30 Sep 2019 13:51:46 +0200
"Laszlo Ersek"  wrote:

> Hi Igor,
> 
> On 09/24/19 13:19, Igor Mammedov wrote:
> > On Mon, 23 Sep 2019 20:35:02 +0200
> > "Laszlo Ersek"  wrote:  
> 
> >> I've got good results. For this (1/2) QEMU patch:
> >>
> >> Tested-by: Laszlo Ersek 
> >>
> >> I tested the following scenarios. In every case, I verified the OVMF
> >> log, and also the "info mtree" monitor command's result (i.e. whether
> >> "smbase-blackhole" / "smbase-window" were disabled or enabled).
> >> Mostly, I diffed these text files between the test scenarios (looking
> >> for desired / undesired differences). In the Linux guests, I checked
> >> / compared the dmesg too (wrt. the UEFI memmap).
> >>
> >> - unpatched OVMF (regression test), Fedora guest, normal boot and S3
> >>
> >> - patched OVMF, but feature disabled with "-global
> >>   mch.smbase-smram=off" (another regression test), Fedora guest,
> >>   normal boot and S3
> >>
> >> - patched OVMF, feature enabled, Fedora and various Windows guests
> >>   (win7, win8, win10 families, client/server), normal boot and S3
> >>
> >> - a subset of the above guests, with S3 disabled (-global
> >>   ICH9-LPC.disable_s3=1), and obviously S3 resume not tested
> >>
> >> SEV: used 5.2-ish Linux guest, with S3 disabled (no support under SEV
> >> for that now):
> >>
> >> - unpatched OVMF (regression test), normal boot
> >>
> >> - patched OVMF but feature disabled on the QEMU cmdline (another
> >>   regression test), normal boot
> >>
> >> - patched OVMF, feature enabled, normal boot.
> >>
> >> I plan to post the OVMF patches tomorrow, for discussion.
> >>
> >> (It's likely too early to push these QEMU / edk2 patches right now --
> >> we don't know yet if this path will take us to the destination. For
> >> now, it certainly looks great.)  
> >
> > Laszlo, thanks for trying it out.
> > It's nice to hear that approach is somewhat usable.
> > Hopefully we won't have to invent 'paused' cpu mode.
> >
> > Pls CC me on your patches
> > (not that I qualify for reviewing,
> > but may be I could learn a thing or two from it)  
> 
> Considering the plan at [1], the two patch sets [2] [3] should cover
> step (01); at least as proof of concept.
> 
> [1] [edk2-devel] CPU hotplug using SMM with QEMU+OVMF
> http://mid.mail-archive.com/20190830164802.1b17ff26@redhat.com
> 
> [2] The current thread:
> [Qemu-devel] [PATCH 0/2] q35: mch: allow to lock down 128K RAM at default 
> SMBASE address
> http://mid.mail-archive.com/20190917130708.10281-1-imammedo@redhat.com
> 
> [3] [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default 
> SMBASE" feature
> http://mid.mail-archive.com/20190924113505.27272-1-lersek@redhat.com
> 
> (I'll have to figure out what SMI handler to put in place there, but I'd
> like to experiment with that once we can cause a new CPU to start
> executing code there, in SMM.)
> 
> So what's next?
> 
> To me it looks like we need to figure out how QEMU can make the OS call
> into SMM (in the GPE cpu hotplug handler), passing in parameters and
> such. This would be step (03).
> 
> Do you agree?
> 
> If so, I'll ask Jiewen about such OS->SMM calls separately, because I
> seem to remember that there used to be an "SMM communcation table" of
> sorts, for flexible OS->SMM calls. However, it appears to be deprecated
> lately.
we can try to resurrect and put over it some kind of protocol
to describe which CPUs to where hotplugged.

or we could put a parameter into SMI status register (IO port 0xb3)
and the trigger SMI from GPE handler to tell SMI handler that cpu
hotplug happened and then use QEMU's cpu hotplug interface
to enumerate hotplugged CPUs for SMI handler.

The later is probably simpler as we won't need to reinvent the wheel
(just reuse the interface that's already in use by GPE handler).

> Hmmm Yes, UEFI 2.8 has "Appendix O - UEFI ACPI Data Table", and it
> writes (after defining the table format):
> 
> The first use of this UEFI ACPI table format is the SMM
> Communication ACPI Table. This table describes a special software
> SMI that can be used to initiate inter-mode communication in the OS
> present environment by non-firmware agents with SMM code.
> 
> Note: The use of the SMM Communication ACPI table is deprecated in
>   UEFI spec. 2.7. This is due to the lack of a use case for
>   inter-mode communication by non-firmware agents with SMM code
>   and support for initiating this form of communication in
>   common OSes.
> 
> The changelog at the front of the UEFI spec also references the
> Mantis#1691 spec ticket, "Remove/Deprecate SMM Communication ACPI Table"
> (addressed in UEFI 2.6B).
> 
> (I think that must have been a security ticket, because, while I
> generally have access to Mantis tickets,
>  gives me "Access
> Denied" :/ )
> 
> Thanks,
> Laszlo
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Groups.io Links: You receive all messages sent to 

Re: [PATCH 01/18] iotests: Filter refcount_order in 036

[Max Reitz]

On 29.09.19 18:31, Maxim Levitsky wrote:
> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
>> This test can run just fine with other values for refcount_bits, so we
>> should filter the value from qcow2.py's dump-header.
>>
>> (036 currently ignores user-specified image options, but that will be
>> fixed in the next patch.)
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/036 | 9 ++---
>>  tests/qemu-iotests/036.out | 6 +++---
>>  2 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
>> index f06ff67408..69d0f9f903 100755
>> --- a/tests/qemu-iotests/036
>> +++ b/tests/qemu-iotests/036
>> @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 
>> 63
>>  
>>  # Without feature table
>>  $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857
>> -$PYTHON qcow2.py "$TEST_IMG" dump-header
>> +$PYTHON qcow2.py "$TEST_IMG" dump-header \
>> +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
>>  _img_info
>>  
>>  # With feature table containing bit 63
>> @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear feature 
>> bit ===
>>  echo
>>  _make_test_img 64M
>>  $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63
>> -$PYTHON qcow2.py "$TEST_IMG" dump-header
>> +$PYTHON qcow2.py "$TEST_IMG" dump-header \
>> +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
>>  
>>  echo
>>  echo === Repair image ===
>>  echo
>>  _check_test_img -r all
>>  
>> -$PYTHON qcow2.py "$TEST_IMG" dump-header
>> +$PYTHON qcow2.py "$TEST_IMG" dump-header \
>> +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
>>  
>>  # success, all done
>>  echo "*** done"
>> diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
>> index e489b44386..998c2a8a35 100644
>> --- a/tests/qemu-iotests/036.out
>> +++ b/tests/qemu-iotests/036.out
>> @@ -19,7 +19,7 @@ snapshot_offset   0x0
>>  incompatible_features 0x8000
>>  compatible_features   0x0
>>  autoclear_features0x0
>> -refcount_order4
>> +refcount_order(filtered)
>>  header_length 104
>>  
>>  qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT 
>> feature(s): Unknown incompatible feature: 8000
>> @@ -53,7 +53,7 @@ snapshot_offset   0x0
>>  incompatible_features 0x0
>>  compatible_features   0x0
>>  autoclear_features0x8000
>> -refcount_order4
>> +refcount_order(filtered)
>>  header_length 104
>>  
>>  Header extension:
>> @@ -81,7 +81,7 @@ snapshot_offset   0x0
>>  incompatible_features 0x0
>>  compatible_features   0x0
>>  autoclear_features0x0
>> -refcount_order4
>> +refcount_order(filtered)
>>  header_length 104
>>  
>>  Header extension:
> 
> Minor notes:
> 
> 1. Maybe put the sed command into a function to avoid duplication?

Hm, maybe, but that would increase the LoC, so I’m not sure whether it
really would be a simplification.

> 2. As I understand the test, it only checks the feature bits.
>So maybe instead of blacklisting some of the values, white list
>only the feature bits in the output?

Hm.  Seems reasonable, but then again I’d prefer a minimal change, and
changing it to a whitelist would be more change...

Max



signature.asc
Description: OpenPGP digital signature

[PATCH 3/5] target/tricore: Raise EXCP_DEBUG in gen_goto_tb() for singlestep

[Bastian Koppelmann]

this is needed for remote gdb connections.

Signed-off-by: Bastian Koppelmann 
---
 target/tricore/translate.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 832ff6328d..6c6303795d 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -3238,6 +3238,14 @@ static inline bool use_goto_tb(DisasContext *ctx, 
target_ulong dest)
 #endif
 }
 
+static void generate_qemu_excp(DisasContext *ctx, int excp)
+{
+TCGv_i32 tmp = tcg_const_i32(excp);
+gen_helper_qemu_excp(cpu_env, tmp);
+ctx->base.is_jmp = DISAS_NORETURN;
+tcg_temp_free(tmp);
+}
+
 static inline void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
 {
 if (use_goto_tb(ctx, dest)) {
@@ -3247,7 +3255,7 @@ static inline void gen_goto_tb(DisasContext *ctx, int n, 
target_ulong dest)
 } else {
 gen_save_pc(dest);
 if (ctx->base.singlestep_enabled) {
-/* raise exception debug */
+generate_qemu_excp(ctx, EXCP_DEBUG);
 }
 tcg_gen_exit_tb(NULL, 0);
 }
@@ -3266,14 +3274,6 @@ static void generate_trap(DisasContext *ctx, int class, 
int tin)
 tcg_temp_free(tintemp);
 }
 
-static void generate_qemu_excp(DisasContext *ctx, int excp)
-{
-TCGv_i32 tmp = tcg_const_i32(excp);
-gen_helper_qemu_excp(cpu_env, tmp);
-ctx->base.is_jmp = DISAS_NORETURN;
-tcg_temp_free(tmp);
-}
-
 static inline void gen_branch_cond(DisasContext *ctx, TCGCond cond, TCGv r1,
TCGv r2, int16_t address)
 {
-- 
2.23.0

[PATCH 1/5] target/tricore: Don't save pc in generate_qemu_excp

[Bastian Koppelmann]

EXCP_DEBUG is the only user. If we encounter a jump in tricore-gdb it's
target was overwritten by generate_qemu_excp() and we would never leave.

Signed-off-by: Bastian Koppelmann 
---
 target/tricore/translate.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index c574638c9f..c556e9a7ab 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -3264,7 +3264,6 @@ static void generate_trap(DisasContext *ctx, int class, 
int tin)
 static void generate_qemu_excp(DisasContext *ctx, int excp)
 {
 TCGv_i32 tmp = tcg_const_i32(excp);
-gen_save_pc(ctx->base.pc_next);
 gen_helper_qemu_excp(cpu_env, tmp);
 ctx->base.is_jmp = DISAS_NORETURN;
 tcg_temp_free(tmp);
-- 
2.23.0

[PATCH 0/5] TriCore fixes and gdbstub

[Bastian Koppelmann]

Hi,

this series fixes a few TriCore problems:

- Segfault due to non initialized ctx->env ptr (see
  https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03527.html)
  I fixed this by properly detangling any reference of the env pointer in the
  translate functions. (as suggested by Peter Maydell) 

- Unimplemented tricore_cpu_get_phys_page_debug() which lead to a temporary fix
  (see b190f477e29c7cd03a8fee49c96d27f160e3f5b0)

The last patch implements a gdbstub for TriCore.

Cheers,
Bastian

Bastian Koppelmann (5):
  target/tricore: Don't save pc in generate_qemu_excp
  target/tricore: Move translate feature check to ctx
  target/tricore: Raise EXCP_DEBUG in gen_goto_tb() for singlestep
  target/tricore: Implement tricore_cpu_get_phys_page_debug
  target/tricore: Implement gdbstub

 target/tricore/Makefile.objs |   2 +-
 target/tricore/cpu.c |  18 +++--
 target/tricore/cpu.h |   2 +
 target/tricore/gdbstub.c | 138 +++
 target/tricore/helper.c  |  13 
 target/tricore/translate.c   |  79 ++--
 6 files changed, 206 insertions(+), 46 deletions(-)
 create mode 100644 target/tricore/gdbstub.c

-- 
2.23.0

[PATCH 2/5] target/tricore: Move translate feature check to ctx

[Bastian Koppelmann]

this allows us to remove the references to env from ctx. This also fixes
a segfault that was due to the unititalized ctx->env ptr.

Reported-by: Andreas Konopik 
Signed-off-by: Bastian Koppelmann 
---
 target/tricore/translate.c | 60 +-
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index c556e9a7ab..832ff6328d 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -66,14 +66,19 @@ static const char *regnames_d[] = {
 
 typedef struct DisasContext {
 DisasContextBase base;
-CPUTriCoreState *env;
 target_ulong pc_succ_insn;
 uint32_t opcode;
 /* Routine used to access memory */
 int mem_idx;
 uint32_t hflags, saved_hflags;
+uint64_t features;
 } DisasContext;
 
+static int has_feature(DisasContext *ctx, int feature)
+{
+return (ctx->features & (1ULL << feature)) != 0;
+}
+
 enum {
 MODE_LL = 0,
 MODE_LU = 1,
@@ -370,7 +375,7 @@ static void gen_swapmsk(DisasContext *ctx, int reg, TCGv ea)
These makros also specify in which ISA version the csfr was introduced. */
 #define R(ADDRESS, REG, FEATURE) \
 case ADDRESS:\
-if (tricore_feature(ctx->env, FEATURE)) {\
+if (has_feature(ctx, FEATURE)) { \
 tcg_gen_ld_tl(ret, cpu_env, offsetof(CPUTriCoreState, REG)); \
 }\
 break;
@@ -395,7 +400,7 @@ static inline void gen_mfcr(DisasContext *ctx, TCGv ret, 
int32_t offset)
 since no execption occurs */
 #define A(ADDRESS, REG, FEATURE) R(ADDRESS, REG, FEATURE)\
 case ADDRESS:\
-if (tricore_feature(ctx->env, FEATURE)) {\
+if (has_feature(ctx, FEATURE)) { \
 tcg_gen_st_tl(r1, cpu_env, offsetof(CPUTriCoreState, REG));  \
 }\
 break;
@@ -3158,7 +3163,7 @@ gen_dvinit_b(DisasContext *ctx, TCGv rl, TCGv rh, TCGv 
r1, TCGv r2)
 {
 TCGv_i64 ret = tcg_temp_new_i64();
 
-if (!tricore_feature(ctx->env, TRICORE_FEATURE_131)) {
+if (!has_feature(ctx, TRICORE_FEATURE_131)) {
 gen_helper_dvinit_b_13(ret, cpu_env, r1, r2);
 } else {
 gen_helper_dvinit_b_131(ret, cpu_env, r1, r2);
@@ -3173,7 +3178,7 @@ gen_dvinit_h(DisasContext *ctx, TCGv rl, TCGv rh, TCGv 
r1, TCGv r2)
 {
 TCGv_i64 ret = tcg_temp_new_i64();
 
-if (!tricore_feature(ctx->env, TRICORE_FEATURE_131)) {
+if (!has_feature(ctx, TRICORE_FEATURE_131)) {
 gen_helper_dvinit_h_13(ret, cpu_env, r1, r2);
 } else {
 gen_helper_dvinit_h_131(ret, cpu_env, r1, r2);
@@ -3655,7 +3660,7 @@ static void decode_src_opc(DisasContext *ctx, int op1)
 tcg_gen_movi_tl(cpu_gpr_a[r1], const4);
 break;
 case OPC1_16_SRC_MOV_E:
-if (tricore_feature(ctx->env, TRICORE_FEATURE_16)) {
+if (has_feature(ctx, TRICORE_FEATURE_16)) {
 tcg_gen_movi_tl(cpu_gpr_d[r1], const4);
 tcg_gen_sari_tl(cpu_gpr_d[r1+1], cpu_gpr_d[r1], 31);
 } else {
@@ -4106,7 +4111,7 @@ static void decode_16Bit_opc(DisasContext *ctx)
 break;
 case OPC1_16_SBC_JEQ2:
 case OPC1_16_SBC_JNE2:
-if (tricore_feature(ctx->env, TRICORE_FEATURE_16)) {
+if (has_feature(ctx, TRICORE_FEATURE_16)) {
 address = MASK_OP_SBC_DISP4(ctx->opcode);
 const16 = MASK_OP_SBC_CONST4_SEXT(ctx->opcode);
 gen_compute_branch(ctx, op1, 0, 0, const16, address);
@@ -4124,7 +4129,7 @@ static void decode_16Bit_opc(DisasContext *ctx)
 /* SBR-format */
 case OPC1_16_SBR_JEQ2:
 case OPC1_16_SBR_JNE2:
-if (tricore_feature(ctx->env, TRICORE_FEATURE_16)) {
+if (has_feature(ctx, TRICORE_FEATURE_16)) {
 r1 = MASK_OP_SBR_S2(ctx->opcode);
 address = MASK_OP_SBR_DISP4(ctx->opcode);
 gen_compute_branch(ctx, op1, r1, 0, 0, address);
@@ -4704,13 +4709,13 @@ static void 
decode_bo_addrmode_post_pre_base(DisasContext *ctx)
 break;
 case OPC2_32_BO_CACHEI_WI_SHORTOFF:
 case OPC2_32_BO_CACHEI_W_SHORTOFF:
-if (!tricore_feature(ctx->env, TRICORE_FEATURE_131)) {
+if (!has_feature(ctx, TRICORE_FEATURE_131)) {
 generate_trap(ctx, TRAPC_INSN_ERR, TIN2_IOPC);
 }
 break;
 case OPC2_32_BO_CACHEI_W_POSTINC:
 case OPC2_32_BO_CACHEI_WI_POSTINC:
-if (tricore_feature(ctx->env, TRICORE_FEATURE_131)) {
+if (has_feature(ctx, TRICORE_FEATURE_131)) {
 tcg_gen_addi_tl(cpu_gpr_a[r2], cpu_gpr_a[r2], off10);
 } else {
 generate_trap(ctx, TRAPC_INSN_ERR

[PATCH 5/5] target/tricore: Implement gdbstub

[Bastian Koppelmann]

Signed-off-by: Bastian Koppelmann 
---
 target/tricore/Makefile.objs |   2 +-
 target/tricore/cpu.c |  10 +++
 target/tricore/cpu.h |   2 +
 target/tricore/gdbstub.c | 138 +++
 4 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 target/tricore/gdbstub.c

diff --git a/target/tricore/Makefile.objs b/target/tricore/Makefile.objs
index 7a05670718..281b55f08d 100644
--- a/target/tricore/Makefile.objs
+++ b/target/tricore/Makefile.objs
@@ -1 +1 @@
-obj-y += translate.o helper.o cpu.o op_helper.o fpu_helper.o
+obj-y += translate.o helper.o cpu.o op_helper.o fpu_helper.o gdbstub.o
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index 7159d9cf8f..fee2f0f5d7 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -28,6 +28,11 @@ static inline void set_feature(CPUTriCoreState *env, int 
feature)
 env->features |= 1ULL << feature;
 }
 
+static gchar *tricore_gdb_arch_name(CPUState *cs)
+{
+return g_strdup("tricore");
+}
+
 static void tricore_cpu_set_pc(CPUState *cs, vaddr value)
 {
 TriCoreCPU *cpu = TRICORE_CPU(cs);
@@ -150,6 +155,11 @@ static void tricore_cpu_class_init(ObjectClass *c, void 
*data)
 cc->class_by_name = tricore_cpu_class_by_name;
 cc->has_work = tricore_cpu_has_work;
 
+cc->gdb_read_register = tricore_cpu_gdb_read_register;
+cc->gdb_write_register = tricore_cpu_gdb_write_register;
+cc->gdb_num_core_regs = 44;
+cc->gdb_arch_name = tricore_gdb_arch_name;
+
 cc->dump_state = tricore_cpu_dump_state;
 cc->set_pc = tricore_cpu_set_pc;
 cc->synchronize_from_tb = tricore_cpu_synchronize_from_tb;
diff --git a/target/tricore/cpu.h b/target/tricore/cpu.h
index 8c014fad07..49d282f728 100644
--- a/target/tricore/cpu.h
+++ b/target/tricore/cpu.h
@@ -353,6 +353,8 @@ enum {
 
 uint32_t psw_read(CPUTriCoreState *env);
 void psw_write(CPUTriCoreState *env, uint32_t val);
+int tricore_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n);
+int tricore_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n);
 
 void fpu_set_state(CPUTriCoreState *env);
 
diff --git a/target/tricore/gdbstub.c b/target/tricore/gdbstub.c
new file mode 100644
index 00..87761f20e9
--- /dev/null
+++ b/target/tricore/gdbstub.c
@@ -0,0 +1,138 @@
+/*
+ * TriCore gdb server stub
+ *
+ * Copyright (c) 2019 Bastian Koppelmann, Paderborn University
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "exec/gdbstub.h"
+
+
+#define LCX_REGNUM 32
+#define FCX_REGNUM 33
+#define PCXI_REGNUM34
+#define TRICORE_PSW_REGNUM 35
+#define TRICORE_PC_REGNUM  36
+#define ICR_REGNUM 37
+#define ISP_REGNUM 38
+#define BTV_REGNUM 39
+#define BIV_REGNUM 40
+#define SYSCON_REGNUM  41
+#define PMUCON0_REGNUM 42
+#define DMUCON_REGNUM  43
+
+static uint32_t tricore_cpu_gdb_read_csfr(CPUTriCoreState *env, int n)
+{
+switch (n) {
+case LCX_REGNUM:
+return env->LCX;
+case FCX_REGNUM:
+return env->FCX;
+case PCXI_REGNUM:
+return env->PCXI;
+case TRICORE_PSW_REGNUM:
+return psw_read(env);
+case TRICORE_PC_REGNUM:
+return env->PC;
+case ICR_REGNUM:
+return env->ICR;
+case ISP_REGNUM:
+return env->ISP;
+case BTV_REGNUM:
+return env->BTV;
+case BIV_REGNUM:
+return env->BIV;
+case SYSCON_REGNUM:
+return env->SYSCON;
+case PMUCON0_REGNUM:
+return 0; /* PMUCON0 */
+case DMUCON_REGNUM:
+return 0; /* DMUCON0 */
+default:
+return 0;
+}
+}
+
+static void tricore_cpu_gdb_write_csfr(CPUTriCoreState *env, int n, uint32_t 
val)
+{
+switch (n) {
+case LCX_REGNUM:
+env->LCX = val;
+break;
+case FCX_REGNUM:
+env->FCX = val;
+break;
+case PCXI_REGNUM:
+env->PCXI = val;
+break;
+case TRICORE_PSW_REGNUM:
+psw_write(env, val);
+break;
+case TRICORE_PC_REGNUM:
+env->PC = val;
+break;
+case ICR_REGNUM:
+env->ICR = val;
+break;
+case ISP_REGNUM:
+env->ISP = val;
+break;
+case BTV_REGNUM:
+env->BTV = val;
+break;
+case BIV_REGNUM:
+env->BIV = val;
+brea

[PATCH 4/5] target/tricore: Implement tricore_cpu_get_phys_page_debug

[Bastian Koppelmann]

this also removes tricore_cpu_get_phys_page_attrs_debug() as it was a
temporary fix from b190f477e29c7cd03a8fee49c96d27f160e3f5b0.

Signed-off-by: Bastian Koppelmann 
---
 target/tricore/cpu.c| 10 +-
 target/tricore/helper.c | 13 +
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index df807c1d74..7159d9cf8f 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -23,14 +23,6 @@
 #include "exec/exec-all.h"
 #include "qemu/error-report.h"
 
-static hwaddr tricore_cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
- MemTxAttrs *attrs)
-{
-error_report("function cpu_get_phys_page_attrs_debug not "
-"implemented, aborting");
-return -1;
-}
-
 static inline void set_feature(CPUTriCoreState *env, int feature)
 {
 env->features |= 1ULL << feature;
@@ -161,7 +153,7 @@ static void tricore_cpu_class_init(ObjectClass *c, void 
*data)
 cc->dump_state = tricore_cpu_dump_state;
 cc->set_pc = tricore_cpu_set_pc;
 cc->synchronize_from_tb = tricore_cpu_synchronize_from_tb;
-cc->get_phys_page_attrs_debug = tricore_cpu_get_phys_page_attrs_debug;
+cc->get_phys_page_debug = tricore_cpu_get_phys_page_debug;
 cc->tcg_initialize = tricore_tcg_init;
 cc->tlb_fill = tricore_cpu_tlb_fill;
 }
diff --git a/target/tricore/helper.c b/target/tricore/helper.c
index d5db7b2c03..7715293263 100644
--- a/target/tricore/helper.c
+++ b/target/tricore/helper.c
@@ -42,6 +42,19 @@ static int get_physical_address(CPUTriCoreState *env, hwaddr 
*physical,
 
 return ret;
 }
+
+hwaddr tricore_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
+{
+TriCoreCPU *cpu = TRICORE_CPU(cs);
+hwaddr phys_addr;
+int prot;
+int mmu_idx = cpu_mmu_index(&cpu->env, false);
+
+if (get_physical_address(&cpu->env, &phys_addr, &prot, addr, 0, mmu_idx)) {
+return -1;
+}
+return phys_addr;
+}
 #endif
 
 /* TODO: Add exeption support*/
-- 
2.23.0

Re: more automated/public CI for QEMU pullreqs

[Peter Maydell]

On Thu, 22 Aug 2019 at 11:50, Stefan Hajnoczi  wrote:
> Thanks for writing up this summary!  The requirements and possible
> solutions are now clear.
>
> We need someone willing to set up and maintain the CI.
>
> One-off tasks:
>
> 1. Create CI runners that offer similar cross-architecture coverage to
>Peter's current setup.  qemu.org has some x86, ppc, and s390 server
>resources available.  I'm not sure about ARM and other architectures.
>
> 2. Write CI configuration to run Peter's "make && make check && make
>check-tcg && linux-user-test".
>
> 3. Document the CI on wiki.qemu.org.
>
> Ongoing responsibilities:
>
> 1. Triage failures that the qemu.git maintainer thinks are related to CI
>runners.
>
> 2. Keep the CI up-to-date and runners online.
>
> Any volunteers?

I see this call for volunteers didn't attract any offers :-/

Thread consensus seems to be toward using gitlab CI -- assuming
we can hand-build the gitlab runner for the architectures
we need without having to block on gitlab fixing their weird
"build for all architectures at once" packaging ?

thanks
-- PMM

Re: [PATCH 02/18] iotests: Replace IMGOPTS by _unsupported_imgopts

[Max Reitz]

On 29.09.19 18:31, Maxim Levitsky wrote:
> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
>> Some tests require compat=1.1 and thus set IMGOPTS='compat=1.1'
>> globally.  That is not how it should be done; instead, they should
>> simply set _unsupported_imgopts to compat=0.10 (compat=1.1 is the
>> default anyway).
>>
>> This makes the tests heed user-specified $IMGOPTS.  Some do not work
>> with all image options, though, so we need to disable them accordingly.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/036 | 3 +--
>>  tests/qemu-iotests/060 | 4 ++--
>>  tests/qemu-iotests/062 | 3 ++-
>>  tests/qemu-iotests/066 | 3 ++-
>>  tests/qemu-iotests/068 | 3 ++-
>>  tests/qemu-iotests/098 | 3 +--
>>  6 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
>> index 69d0f9f903..57dda23b02 100755
>> --- a/tests/qemu-iotests/036
>> +++ b/tests/qemu-iotests/036
>> @@ -43,9 +43,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  # This tests qcow2-specific low-level functionality
>>  _supported_fmt qcow2
>>  _supported_proto file
>> -
>>  # Only qcow2v3 and later supports feature bits
>> -IMGOPTS="compat=1.1"
>> +_unsupported_imgopts 'compat=0.10'
>>  
>>  echo
>>  echo === Image with unknown incompatible feature bit ===
>> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
>> index b91d8321bb..9c2ef42522 100755
>> --- a/tests/qemu-iotests/060
>> +++ b/tests/qemu-iotests/060
>> @@ -48,6 +48,8 @@ _filter_io_error()
>>  _supported_fmt qcow2
>>  _supported_proto file
>>  _supported_os Linux
>> +# These tests only work for compat=1.1 images with refcount_bits=16
>> +_unsupported_imgopts 'compat=0.10' 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
>>  
>>  rt_offset=65536  # 0x1 (XXX: just an assumption)
>>  rb_offset=131072 # 0x2 (XXX: just an assumption)
>> @@ -55,8 +57,6 @@ l1_offset=196608 # 0x3 (XXX: just an assumption)
>>  l2_offset=262144 # 0x4 (XXX: just an assumption)
>>  l2_offset_after_snapshot=524288 # 0x8 (XXX: just an assumption)
>>  
>> -IMGOPTS="compat=1.1"
>> -
>>  OPEN_RW="open -o overlap-check=all $TEST_IMG"
>>  # Overlap checks are done before write operations only, therefore opening an
>>  # image read-only makes the overlap-check option irrelevant
>> diff --git a/tests/qemu-iotests/062 b/tests/qemu-iotests/062
>> index d5f818fcce..ac0d2a9a3b 100755
>> --- a/tests/qemu-iotests/062
>> +++ b/tests/qemu-iotests/062
>> @@ -40,8 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  # This tests qocw2-specific low-level functionality
>>  _supported_fmt qcow2
>>  _supported_proto generic
>> +# We need zero clusters and snapshots
>> +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
>>  
>> -IMGOPTS="compat=1.1"
>>  IMG_SIZE=64M
>>  
>>  echo
>> diff --git a/tests/qemu-iotests/066 b/tests/qemu-iotests/066
>> index 28f8c98412..9a15ba8027 100755
>> --- a/tests/qemu-iotests/066
>> +++ b/tests/qemu-iotests/066
>> @@ -39,9 +39,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  # This tests qocw2-specific low-level functionality
>>  _supported_fmt qcow2
>>  _supported_proto generic
>> +# Weneed zero clusters and snapshots
> Typo

Indeed!

>> +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
>>  
>>  # Intentionally create an unaligned image
>> -IMGOPTS="compat=1.1"
>>  IMG_SIZE=$((64 * 1024 * 1024 + 512))
>>  
>>  echo
>> diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
>> index 22f5ca3ba6..65650fca9a 100755
>> --- a/tests/qemu-iotests/068
>> +++ b/tests/qemu-iotests/068
>> @@ -39,8 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  # This tests qocw2-specific low-level functionality
>>  _supported_fmt qcow2
>>  _supported_proto generic
>> +# Internal snapshots are (currently) impossible with refcount_bits=1
> Why currently? 1 bit will only allow to mark a cluser as used/unused which
> is not enough for any snapshots?

It is, because in theory you can just copy the clusters at the time of
snapshotting.  We’ll never implement it, but, well...

>> +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
>>  
>> -IMGOPTS="compat=1.1"
>>  IMG_SIZE=128K
>>  
>>  case "$QEMU_DEFAULT_MACHINE" in
>> diff --git a/tests/qemu-iotests/098 b/tests/qemu-iotests/098
>> index 1c1d1c468f..2d68dc7d6c 100755
>> --- a/tests/qemu-iotests/098
>> +++ b/tests/qemu-iotests/098
>> @@ -40,8 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  
>>  _supported_fmt qcow2
>>  _supported_proto file
>> -
>> -IMGOPTS="compat=1.1"
>> +_unsupported_imgopts 'compat=0.10'
> Any idea why? I am not familiar with qcow2 well enought to
> know but this misses a comment with justification.

Because the special bdrv_make_empty() version we want to test only works
with qcow2 v3 images.

>>  
>>  for event in l1_update empty_image_prepare reftable_update refblock_alloc; 
>> do
>>  
> 
> 
> Best regards,
>   Maxim Levitsky
> 

Thanks for reviewing!

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 07/18] iotests: Replace IMGOPTS= by -o

[Max Reitz]

On 29.09.19 18:33, Maxim Levitsky wrote:
> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
>> Tests should not overwrite all user-supplied image options, but only add
>> to it (which will effectively overwrite conflicting values).  Accomplish
>> this by passing options to _make_test_img via -o instead of $IMGOPTS.
>>
>> For some tests, there is no functional change because they already only
>> appended options to IMGOPTS.  For these, this patch is just a
>> simplification.
>>
>> For others, this is a change, so they now heed user-specified $IMGOPTS.
>> Some of those tests do not work with all image options, though, so we
>> need to disable them accordingly.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/031 |  9 ---
>>  tests/qemu-iotests/039 | 24 ++
>>  tests/qemu-iotests/059 | 18 ++---
>>  tests/qemu-iotests/060 |  6 ++---
>>  tests/qemu-iotests/061 | 57 ++
>>  tests/qemu-iotests/079 |  3 +--
>>  tests/qemu-iotests/106 |  2 +-
>>  tests/qemu-iotests/108 |  2 +-
>>  tests/qemu-iotests/112 | 32 
>>  tests/qemu-iotests/115 |  3 +--
>>  tests/qemu-iotests/121 |  6 ++---
>>  tests/qemu-iotests/125 |  2 +-
>>  tests/qemu-iotests/137 |  2 +-
>>  tests/qemu-iotests/138 |  3 +--
>>  tests/qemu-iotests/175 |  2 +-
>>  tests/qemu-iotests/190 |  2 +-
>>  tests/qemu-iotests/191 |  3 +--
>>  tests/qemu-iotests/220 |  4 ++-
>>  tests/qemu-iotests/243 |  6 +++--
>>  tests/qemu-iotests/244 | 10 +---
>>  tests/qemu-iotests/250 |  3 +--
>>  tests/qemu-iotests/265 |  2 +-
>>  22 files changed, 100 insertions(+), 101 deletions(-)

[...]

>> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
>> index 325da63a4c..99563bf126 100755
>> --- a/tests/qemu-iotests/039
>> +++ b/tests/qemu-iotests/039
>> @@ -50,8 +50,7 @@ size=128M
>>  echo
>>  echo "== Checking that image is clean on shutdown =="
>>  
>> -IMGOPTS="compat=1.1,lazy_refcounts=on"
>> -_make_test_img $size
>> +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size
> Any reason for compat=1.1 here? Other that it was before like that.

Lazy refcounts only work with qcow2 v3 (compat=1.1).

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 09/18] iotests: Drop IMGOPTS use in 267

[Max Reitz]

On 29.09.19 18:33, Maxim Levitsky wrote:
> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
>> Overwriting IMGOPTS means ignoring all user-supplied options, which is
>> not what we want.  Replace the current IMGOPTS use by a new BACKING_FILE
>> variable.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/267 | 12 
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
>> index 95f885442f..529f5f9afe 100755
>> --- a/tests/qemu-iotests/267
>> +++ b/tests/qemu-iotests/267
>> @@ -68,7 +68,11 @@ size=128M
>>  
>>  run_test()
>>  {
>> -_make_test_img $size
>> +if [ -n "$BACKING_FILE" ]; then
>> +_make_test_img -b "$BACKING_FILE" $size
>> +else
>> +_make_test_img $size
>> +fi
>>  printf "savevm snap0\ninfo snapshots\nloadvm snap0\n" | run_qemu "$@" | 
>> _filter_date
>>  }
>>  
>> @@ -119,12 +123,12 @@ echo
>>  
>>  TEST_IMG="$TEST_IMG.base" _make_test_img $size
>>  
>> -IMGOPTS="backing_file=$TEST_IMG.base" \
>> +BACKING_FILE="$TEST_IMG.base" \
>>  run_test -blockdev 
>> driver=file,filename="$TEST_IMG.base",node-name=backing-file \
>>   -blockdev driver=file,filename="$TEST_IMG",node-name=file \
>>   -blockdev 
>> driver=$IMGFMT,file=file,backing=backing-file,node-name=fmt
>>  
>> -IMGOPTS="backing_file=$TEST_IMG.base" \
>> +BACKING_FILE="$TEST_IMG.base" \
>>  run_test -blockdev 
>> driver=file,filename="$TEST_IMG.base",node-name=backing-file \
>>   -blockdev driver=$IMGFMT,file=backing-file,node-name=backing-fmt \
>>   -blockdev driver=file,filename="$TEST_IMG",node-name=file \
>> @@ -141,7 +145,7 @@ echo
>>  echo "=== -blockdev with NBD server on the backing file ==="
>>  echo
>>  
>> -IMGOPTS="backing_file=$TEST_IMG.base" _make_test_img $size
>> +_make_test_img -b "$TEST_IMG.base" $size
>>  cat <>  nbd_server_start unix:$TEST_DIR/nbd
>>  nbd_server_add -w backing-fmt
> 
> qemu's master (pulled today), don't have iotest 267,
> you probably based this on some not yet merged branch.
> Or I made some mistake with pulling the master branch.

Yep, it’s only in Kevin’s block branch (and thus mine, too).  He sent a
pull request for it, which was rejected though (because this test is
broken on anything but x64, but that doesn’t stop this patch).

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 0/2] qapi: Add detection for the 'savevm' fix for blockdev

[Peter Krempa]

On Fri, Sep 20, 2019 at 16:26:43 +0200, Peter Krempa wrote:
> Add 'features' field in the schema for commands and add a feature flag
> to advertise that the fix for savevm [1] is present.
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03487.html
> 
> v2:
>  - fixed typos pointed out by Eric
>  - added 'since' tag
>  - reworded to describe the fix this allows to detect properly (Kevin)
>  - verified that this can be rebased on top of Markus' series
>automatically

Ping

Re: [PATCH 13/18] iotests: Make 091 work with data_file

[Max Reitz]

On 29.09.19 18:34, Maxim Levitsky wrote:
> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
>> The image end offset as reported by qemu-img check is different when
>> using an external data file; we do not care about its value here, so we
>> can just filter it.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/091 | 3 ++-
>>  tests/qemu-iotests/091.out | 1 -
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
>> index f4b44659ae..7536ca4607 100755
>> --- a/tests/qemu-iotests/091
>> +++ b/tests/qemu-iotests/091
>> @@ -101,7 +101,8 @@ echo "Check image pattern"
>>  ${QEMU_IO} -c "read -P 0x22 0 4M" "${TEST_IMG}" | _filter_testdir | 
>> _filter_qemu_io
>>  
>>  echo "Running 'qemu-img check -r all \$TEST_IMG'"
>> -"${QEMU_IMG}" check -r all "${TEST_IMG}" 2>&1 | _filter_testdir | 
>> _filter_qemu
>> +"${QEMU_IMG}" check -r all "${TEST_IMG}" 2>&1 | _filter_testdir | 
>> _filter_qemu \
>> +| sed '/Image end offset/d'
> 
> Maybe use _filter_qemu_img_check instead? I see it actually filters this 
> already.

Uh, nice.  Will do, thanks.

Max



signature.asc
Description: OpenPGP digital signature

Lockup with --accel tcg,thread=single

[Doug Gale]

I found a lockup in single threaded TCG, with OVMF bios, 16 CPUs.

qemu-system-x86_64 --accel tcg,thread=single -smp cpus=16 -bios
/usr/share/ovmf/OVMF.fd

Using Ubuntu 18.04 LTS, default gnome desktop. There is no guest OS, there
is no hard drive, just the OVMF firmware locks it up. (I narrowed it down
to this from a much larger repro)

Let that run for about 10 seconds or so, then attempt to Ctrl-C in the
terminal that launched qemu. You should see the hung program thing (wait or
force quit) appear on the qemu window, and the terminal just prints ^C
without interrupting qemu. DO NOT click force quit or wait. Use killall -9
qemu-system-x86_64 to kill it if you must, gdb kill is also okay. If you
force quit you will likely freeze the entire gnome desktop. This is what
kmsg reports:
https://gist.githubusercontent.com/doug65536/810e0471c11008c97edb4aa0d86ddd66/raw/40038a355542f0edb1f14a7c7fb7c2db60da0025/end%2520of%2520syslog%2520after%2520desktop%2520lockup,
this is what syslog reports:
https://gist.githubusercontent.com/doug65536/810e0471c11008c97edb4aa0d86ddd66/raw/40038a355542f0edb1f14a7c7fb7c2db60da0025/the%2520end%2520of%2520dmesg%2520after%2520desktop%2520lockup.
Probably bugs in gnome but just warning about locking up your machines when
reproducing.

Peter Maydell helped me bisect it in IRC.
 Works fine at commit 1e8a98b53867f61
 Fails at commit 9458a9a1df1a4c7

MTTCG works fine all the way up to master.

Configure command line:

../qemu/configure --target-list=x86_64-softmmu,i386-softmmu
--audio-drv-list=pa --enable-libusb --disable-libssh --enable-virglrenderer
--enable-opengl --enable-debug

The issue occurs without --enable-debug. I didn't strip the configure down
though, it may not need all of those configure options exactly.

OVMF from ubuntu package manager, package named ovmf, exact version
is 0~20180205.c0d9813c-2ubuntu0.1

Stack trace at lockup at
https://gist.githubusercontent.com/doug65536/810e0471c11008c97edb4aa0d86ddd66/raw/40038a355542f0edb1f14a7c7fb7c2db60da0025/backtrace%2520all

Lockup with --accel tcg,thread=single

[Doug Gale]

I found a lockup in single threaded TCG, with OVMF bios, 16 CPUs.

qemu-system-x86_64 --accel tcg,thread=single -smp cpus=16 -bios
/usr/share/ovmf/OVMF.fd

Using Ubuntu 18.04 LTS, default gnome desktop. There is no guest OS,
there is no hard drive, just the OVMF firmware locks it up. (I
narrowed it down to this from a much larger repro)

Let that run for about 10 seconds or so, then attempt to Ctrl-C in the
terminal that launched qemu. You should see the hung program thing
(wait or force quit) appear on the qemu window, and the terminal just
prints ^C without interrupting qemu. DO NOT click force quit or wait.
Use killall -9 qemu-system-x86_64 to kill it if you must, gdb kill is
also okay. If you force quit you will likely freeze the entire gnome
desktop. This is what kmsg reports:
https://gist.githubusercontent.com/doug65536/810e0471c11008c97edb4aa0d86ddd66/raw/40038a355542f0edb1f14a7c7fb7c2db60da0025/end%2520of%2520syslog%2520after%2520desktop%2520lockup,
this is what syslog reports:
https://gist.githubusercontent.com/doug65536/810e0471c11008c97edb4aa0d86ddd66/raw/40038a355542f0edb1f14a7c7fb7c2db60da0025/the%2520end%2520of%2520dmesg%2520after%2520desktop%2520lockup.
Probably bugs in gnome but just warning about locking up your machines
when reproducing.

Peter Maydell helped me bisect it in IRC.
 Works fine at commit 1e8a98b53867f61
 Fails at commit 9458a9a1df1a4c7

MTTCG works fine all the way up to master.

Configure command line:

../qemu/configure --target-list=x86_64-softmmu,i386-softmmu
--audio-drv-list=pa --enable-libusb --disable-libssh
--enable-virglrenderer --enable-opengl --enable-debug

The issue occurs without --enable-debug. I didn't strip the configure
down though, it may not need all of those configure options exactly.

OVMF from ubuntu package manager, package named ovmf, exact version is
0~20180205.c0d9813c-2ubuntu0.1

Stack trace at lockup at
https://gist.githubusercontent.com/doug65536/810e0471c11008c97edb4aa0d86ddd66/raw/40038a355542f0edb1f14a7c7fb7c2db60da0025/backtrace%2520all

Re: [RFC PATCH 00/12] Add SDEI support for arm64

[Peter Maydell]

On Tue, 24 Sep 2019 at 16:23, Heyi Guo  wrote:
>
> As promised, this is the first RFC patch set for arm64 SDEI support.

Hi; for the benefit of possible reviewers who aren't familiar
with every corner of the arm ecosystem, could you provide a
summary of:
 * what is SDEI ?
 * what do KVM and QEMU want/need to do with it ?
 * what is this patchset trying to solve ?

That would provide some useful context for trying to
review the patchset.

thanks
-- PMM

[PULL 07/12] configure: Remove s390 (31-bit mode) from the list of supported CPUs

[Christian Borntraeger]

From: Thomas Huth 

On IBM Z, KVM in the kernel is only implemented for 64-bit mode, and
with regards to TCG, we also only support 64-bit host CPUs (see the
check at the beginning of tcg/s390/tcg-target.inc.c), so we should
remove s390 (without "x", i.e. the old 31-bit mode CPUs) from the
list of supported CPUs.

Signed-off-by: Thomas Huth 
Message-Id: <20190928190334.6897-1-th...@redhat.com>
Reviewed-by: David Hildenbrand 
Signed-off-by: Christian Borntraeger 
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 542f6aea3f61..8f8446f52b92 100755
--- a/configure
+++ b/configure
@@ -728,7 +728,7 @@ ARCH=
 # Normalise host CPU name and set ARCH.
 # Note that this case should only have supported host CPUs, not guests.
 case "$cpu" in
-  ppc|ppc64|s390|s390x|sparc64|x32|riscv32|riscv64)
+  ppc|ppc64|s390x|sparc64|x32|riscv32|riscv64)
 supported_cpu="yes"
   ;;
   ppc64le)
-- 
2.21.0

[PULL 05/12] s390x: sclp: fix error handling for oversize control blocks

[Christian Borntraeger]

From: Janosch Frank 

Requests over 4k are not a spec exception.

Signed-off-by: Janosch Frank 
Reviewed-by: Jason J. Herne 
Message-Id: <1569591203-15258-4-git-send-email-imbre...@linux.ibm.com>
Acked-by: David Hildenbrand 
Signed-off-by: Christian Borntraeger 
---
 hw/s390x/sclp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 73244c938b10..abb6e5011f9c 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -213,8 +213,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, 
uint32_t code)
 cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
 
 /* Valid sccb sizes */
-if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader) ||
-be16_to_cpu(work_sccb.h.length) > SCCB_SIZE) {
+if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
 r = -PGM_SPECIFICATION;
 goto out;
 }
-- 
2.21.0

[PULL 11/12] s390: do not call memory_region_allocate_system_memory() multiple times

[Christian Borntraeger]

From: Igor Mammedov 

s390 was trying to solve limited KVM memslot size issue by abusing
memory_region_allocate_system_memory(), which breaks API contract
where the function might be called only once.

Beside an invalid use of API, the approach also introduced migration
issue, since RAM chunks for each KVM_SLOT_MAX_BYTES are transferred in
migration stream as separate RAMBlocks.

After discussion [1], it was agreed to break migration from older
QEMU for guest with RAM >8Tb (as it was relatively new (since 2.12)
and considered to be not actually used downstream).
Migration should keep working for guests with less than 8TB and for
more than 8TB with QEMU 4.2 and newer binary.
In case user tries to migrate more than 8TB guest, between incompatible
QEMU versions, migration should fail gracefully due to non-exiting
RAMBlock ID or RAMBlock size mismatch.

Taking in account above and that now KVM code is able to split too
big MemorySection into several memslots, partially revert commit
 (bb223055b s390-ccw-virtio: allow for systems larger that 7.999TB)
and use kvm_set_max_memslot_size() to set KVMSlot size to
KVM_SLOT_MAX_BYTES.

1) [PATCH RFC v2 4/4] s390: do not call  memory_region_allocate_system_memory() 
multiple times

Signed-off-by: Igor Mammedov 
Message-Id: <20190924144751.24149-5-imamm...@redhat.com>
Acked-by: Peter Xu 
Signed-off-by: Christian Borntraeger 
---
 hw/s390x/s390-virtio-ccw.c | 30 +++---
 target/s390x/kvm.c | 11 +++
 2 files changed, 14 insertions(+), 27 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 8bfb6684cb72..18ad279a00a3 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -154,39 +154,15 @@ static void virtio_ccw_register_hcalls(void)
virtio_ccw_hcall_early_printk);
 }
 
-/*
- * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
- * as the dirty bitmap must be managed by bitops that take an int as
- * position indicator. If we have a guest beyond that we will split off
- * new subregions. The split must happen on a segment boundary (1MB).
- */
-#define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
-#define SEG_MSK (~0xfULL)
-#define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & 
SEG_MSK)
 static void s390_memory_init(ram_addr_t mem_size)
 {
 MemoryRegion *sysmem = get_system_memory();
-ram_addr_t chunk, offset = 0;
-unsigned int number = 0;
+MemoryRegion *ram = g_new(MemoryRegion, 1);
 Error *local_err = NULL;
-gchar *name;
 
 /* allocate RAM for core */
-name = g_strdup_printf("s390.ram");
-while (mem_size) {
-MemoryRegion *ram = g_new(MemoryRegion, 1);
-uint64_t size = mem_size;
-
-/* KVM does not allow memslots >= 8 TB */
-chunk = MIN(size, KVM_SLOT_MAX_BYTES);
-memory_region_allocate_system_memory(ram, NULL, name, chunk);
-memory_region_add_subregion(sysmem, offset, ram);
-mem_size -= chunk;
-offset += chunk;
-g_free(name);
-name = g_strdup_printf("s390.ram.%u", ++number);
-}
-g_free(name);
+memory_region_allocate_system_memory(ram, NULL, "s390.ram", mem_size);
+memory_region_add_subregion(sysmem, 0, ram);
 
 /*
  * Configure the maximum page size. As no memory devices were created
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 97a662ad0ebf..54864c259c5e 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -28,6 +28,7 @@
 #include "cpu.h"
 #include "internal.h"
 #include "kvm_s390x.h"
+#include "sysemu/kvm_int.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
@@ -122,6 +123,15 @@
  */
 #define VCPU_IRQ_BUF_SIZE(max_cpus) (sizeof(struct kvm_s390_irq) * \
  (max_cpus + NR_LOCAL_IRQS))
+/*
+ * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
+ * as the dirty bitmap must be managed by bitops that take an int as
+ * position indicator. If we have a guest beyond that we will split off
+ * new subregions. The split must happen on a segment boundary (1MB).
+ */
+#define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
+#define SEG_MSK (~0xfULL)
+#define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & 
SEG_MSK)
 
 static CPUWatchpoint hw_watchpoint;
 /*
@@ -355,6 +365,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
  */
 /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
 
+kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES);
 return 0;
 }
 
-- 
2.21.0

Re: [RFC PATCH 07/12] arm/sdei: override qemu_irq handler when binding interrupt

[Peter Maydell]

On Tue, 24 Sep 2019 at 16:23, Heyi Guo  wrote:
>
> Override qemu_irq handler to support trigger SDEI event transparently
> after guest binds interrupt to SDEI event. We don't have good way to
> get GIC device and to guarantee SDEI device is initialized after GIC,
> so we search GIC in system bus when the first SDEI request happens or
> in VMSTATE post_load().
>
> Signed-off-by: Heyi Guo 
> Cc: Peter Maydell 
> Cc: Dave Martin 
> Cc: Marc Zyngier 
> Cc: Mark Rutland 
> Cc: James Morse 


> +static void override_qemu_irq(QemuSDEState *s, int32_t event, uint32_t intid)
> +{
> +qemu_irq irq;
> +QemuSDE *sde;
> +CPUState *cs;
> +int cpu;
> +
> +/* SPI */
> +if (intid >= GIC_INTERNAL) {
> +cs = arm_get_cpu_by_id(0);
> +irq = qdev_get_gpio_in(s->gic_dev,
> +   gic_int_to_irq(s->num_irq, intid, 0));
> +if (irq) {
> +qemu_irq_intercept_in(&irq, qemu_sdei_irq_handler, 1);
> +}

I'm not sure what this code is trying to do, but
qemu_irq_intercept_in() is a function for internal use
by the qtest testing infrastructure, so it shouldn't be
used in 'real' QEMU code.

> +sde = get_sde_no_check(s, event, cs);
> +sde->irq = irq;
> +put_sde(sde, cs);
> +return;
> +}

> @@ -1042,6 +1152,17 @@ void sdei_handle_request(CPUState *cs, struct kvm_run 
> *run)
>  return;
>  }
>
> +if (!sde_state->gic_dev) {
> +/* Search for ARM GIC device */
> +qbus_walk_children(sysbus_get_default(), dev_walkerfn,
> +   NULL, NULL, NULL, sde_state);
> +if (!sde_state->gic_dev) {
> +error_report("Cannot find ARM GIC device!");
> +run->hypercall.args[0] = SDEI_NOT_SUPPORTED;
> +return;
> +}
> +}

Walking through the qbus tree looking for particular devices
isn't really something I'd recommend either.

thanks
-- PMM

[PULL 12/12] s390/kvm: split kvm mem slots at 4TB

[Christian Borntraeger]

Instead of splitting at an unaligned address, we can simply split at
4TB.

Signed-off-by: Christian Borntraeger 
Acked-by: Igor Mammedov 
---
 target/s390x/kvm.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 54864c259c5e..c24c869e7703 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -126,12 +126,11 @@
 /*
  * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
  * as the dirty bitmap must be managed by bitops that take an int as
- * position indicator. If we have a guest beyond that we will split off
- * new subregions. The split must happen on a segment boundary (1MB).
+ * position indicator. This would end at an unaligned  address
+ * (0x7f0). As future variants might provide larger pages
+ * and to make all addresses properly aligned, let us split at 4TB.
  */
-#define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
-#define SEG_MSK (~0xfULL)
-#define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & 
SEG_MSK)
+#define KVM_SLOT_MAX_BYTES (4UL * TiB)
 
 static CPUWatchpoint hw_watchpoint;
 /*
-- 
2.21.0

[PULL 10/12] kvm: split too big memory section on several memslots

[Christian Borntraeger]

From: Igor Mammedov 

Max memslot size supported by kvm on s390 is 8Tb,
move logic of splitting RAM in chunks upto 8T to KVM code.

This way it will hide KVM specific restrictions in KVM code
and won't affect board level design decisions. Which would allow
us to avoid misusing memory_region_allocate_system_memory() API
and eventually use a single hostmem backend for guest RAM.

Signed-off-by: Igor Mammedov 
Message-Id: <20190924144751.24149-4-imamm...@redhat.com>
Reviewed-by: Peter Xu 
Acked-by: Paolo Bonzini 
Signed-off-by: Christian Borntraeger 
---
 accel/kvm/kvm-all.c  | 122 +--
 include/sysemu/kvm_int.h |   1 +
 2 files changed, 80 insertions(+), 43 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index ff9b95c0d103..aabe097c410f 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -140,6 +140,7 @@ bool kvm_direct_msi_allowed;
 bool kvm_ioeventfd_any_length_allowed;
 bool kvm_msi_use_devid;
 static bool kvm_immediate_exit;
+static hwaddr kvm_max_slot_size = ~0;
 
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
 KVM_CAP_INFO(USER_MEMORY),
@@ -437,7 +438,7 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, 
KVMSlot *mem,
 static int kvm_section_update_flags(KVMMemoryListener *kml,
 MemoryRegionSection *section)
 {
-hwaddr start_addr, size;
+hwaddr start_addr, size, slot_size;
 KVMSlot *mem;
 int ret = 0;
 
@@ -448,13 +449,18 @@ static int kvm_section_update_flags(KVMMemoryListener 
*kml,
 
 kvm_slots_lock(kml);
 
-mem = kvm_lookup_matching_slot(kml, start_addr, size);
-if (!mem) {
-/* We don't have a slot if we want to trap every access. */
-goto out;
-}
+while (size && !ret) {
+slot_size = MIN(kvm_max_slot_size, size);
+mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
+if (!mem) {
+/* We don't have a slot if we want to trap every access. */
+goto out;
+}
 
-ret = kvm_slot_update_flags(kml, mem, section->mr);
+ret = kvm_slot_update_flags(kml, mem, section->mr);
+start_addr += slot_size;
+size -= slot_size;
+}
 
 out:
 kvm_slots_unlock(kml);
@@ -527,11 +533,15 @@ static int 
kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
 struct kvm_dirty_log d = {};
 KVMSlot *mem;
 hwaddr start_addr, size;
+hwaddr slot_size, slot_offset = 0;
 int ret = 0;
 
 size = kvm_align_section(section, &start_addr);
-if (size) {
-mem = kvm_lookup_matching_slot(kml, start_addr, size);
+while (size) {
+MemoryRegionSection subsection = *section;
+
+slot_size = MIN(kvm_max_slot_size, size);
+mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
 if (!mem) {
 /* We don't have a slot if we want to trap every access. */
 goto out;
@@ -549,11 +559,11 @@ static int 
kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
  * So for now, let's align to 64 instead of HOST_LONG_BITS here, in
  * a hope that sizeof(long) won't become >8 any time soon.
  */
-size = ALIGN(((mem->memory_size) >> TARGET_PAGE_BITS),
- /*HOST_LONG_BITS*/ 64) / 8;
 if (!mem->dirty_bmap) {
+hwaddr bitmap_size = ALIGN(((mem->memory_size) >> 
TARGET_PAGE_BITS),
+/*HOST_LONG_BITS*/ 64) / 8;
 /* Allocate on the first log_sync, once and for all */
-mem->dirty_bmap = g_malloc0(size);
+mem->dirty_bmap = g_malloc0(bitmap_size);
 }
 
 d.dirty_bitmap = mem->dirty_bmap;
@@ -564,7 +574,13 @@ static int 
kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
 goto out;
 }
 
-kvm_get_dirty_pages_log_range(section, d.dirty_bitmap);
+subsection.offset_within_region += slot_offset;
+subsection.size = int128_make64(slot_size);
+kvm_get_dirty_pages_log_range(&subsection, d.dirty_bitmap);
+
+slot_offset += slot_size;
+start_addr += slot_size;
+size -= slot_size;
 }
 out:
 return ret;
@@ -972,6 +988,14 @@ kvm_check_extension_list(KVMState *s, const 
KVMCapabilityInfo *list)
 return NULL;
 }
 
+void kvm_set_max_memslot_size(hwaddr max_slot_size)
+{
+g_assert(
+ROUND_UP(max_slot_size, qemu_real_host_page_size) == max_slot_size
+);
+kvm_max_slot_size = max_slot_size;
+}
+
 static void kvm_set_phys_mem(KVMMemoryListener *kml,
  MemoryRegionSection *section, bool add)
 {
@@ -979,7 +1003,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
 int err;
 MemoryRegion *mr = section->mr;
 bool writeable = !mr->readonly && !mr->rom_device;
-hwaddr start_addr, size;
+hwaddr start_addr, size, slot_size;
 void *ram;
 
 if (!memory_region_is_ram(mr)) {
@@ -1004,41 +1028,52 @@ static void k

[PULL 01/12] MAINTAINERS: Update S390 PCI Maintainer

[Christian Borntraeger]

From: Matthew Rosato 

As discussed previously with Collin, I will take over maintaining
s390 pci.

Signed-off-by: Matthew Rosato 
Message-Id: <1569590461-12562-1-git-send-email-mjros...@linux.ibm.com>
Acked-by: Collin Walling 
Signed-off-by: Christian Borntraeger 
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index bd7ee2310184..21264eae9c43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1206,7 +1206,7 @@ T: git https://github.com/borntraeger/qemu.git s390-next
 L: qemu-s3...@nongnu.org
 
 S390 PCI
-M: Collin Walling 
+M: Matthew Rosato 
 S: Supported
 F: hw/s390x/s390-pci*
 L: qemu-s3...@nongnu.org
-- 
2.21.0

[PULL 09/12] kvm: clear dirty bitmaps from all overlapping memslots

[Christian Borntraeger]

From: Paolo Bonzini 

Currently MemoryRegionSection has 1:1 mapping to KVMSlot.
However next patch will allow splitting MemoryRegionSection into
several KVMSlot-s, make sure that kvm_physical_log_slot_clear()
is able to handle such 1:N mapping.

Signed-off-by: Paolo Bonzini 
Signed-off-by: Igor Mammedov 
Reviewed-by: Peter Xu 
Message-Id: <20190924144751.24149-3-imamm...@redhat.com>
Acked-by: Paolo Bonzini 
Signed-off-by: Christian Borntraeger 
---
 accel/kvm/kvm-all.c | 36 ++--
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index a85ec09486dd..ff9b95c0d103 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -589,8 +589,8 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, 
uint64_t start,
  * satisfy the KVM interface requirement.  Firstly, do the start
  * page alignment on 64 host pages
  */
-bmap_start = (start - mem->start_addr) & KVM_CLEAR_LOG_MASK;
-start_delta = start - mem->start_addr - bmap_start;
+bmap_start = start & KVM_CLEAR_LOG_MASK;
+start_delta = start - bmap_start;
 bmap_start /= psize;
 
 /*
@@ -694,8 +694,8 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
   MemoryRegionSection *section)
 {
 KVMState *s = kvm_state;
-uint64_t start, size;
-KVMSlot *mem = NULL;
+uint64_t start, size, offset, count;
+KVMSlot *mem;
 int ret, i;
 
 if (!s->manual_dirty_log_protect) {
@@ -713,22 +713,30 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
 
 kvm_slots_lock(kml);
 
-/* Find any possible slot that covers the section */
 for (i = 0; i < s->nr_slots; i++) {
 mem = &kml->slots[i];
-if (mem->start_addr <= start &&
-start + size <= mem->start_addr + mem->memory_size) {
+/* Discard slots that are empty or do not overlap the section */
+if (!mem->memory_size ||
+mem->start_addr > start + size - 1 ||
+start > mem->start_addr + mem->memory_size - 1) {
+continue;
+}
+
+if (start >= mem->start_addr) {
+/* The slot starts before section or is aligned to it.  */
+offset = start - mem->start_addr;
+count = MIN(mem->memory_size - offset, size);
+} else {
+/* The slot starts after section.  */
+offset = 0;
+count = MIN(mem->memory_size, size - (mem->start_addr - start));
+}
+ret = kvm_log_clear_one_slot(mem, kml->as_id, offset, count);
+if (ret < 0) {
 break;
 }
 }
 
-/*
- * We should always find one memslot until this point, otherwise
- * there could be something wrong from the upper layer
- */
-assert(mem && i != s->nr_slots);
-ret = kvm_log_clear_one_slot(mem, kml->as_id, start, size);
-
 kvm_slots_unlock(kml);
 
 return ret;
-- 
2.21.0

[PULL 04/12] s390x: sclp: boundary check

[Christian Borntraeger]

From: Janosch Frank 

All sclp codes need to be checked for page boundary violations.

Signed-off-by: Janosch Frank 
Reviewed-by: Jason J. Herne 
Message-Id: <1569591203-15258-3-git-send-email-imbre...@linux.ibm.com>
Reviewed-by: David Hildenbrand 
Signed-off-by: Christian Borntraeger 
---
 hw/s390x/sclp.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 95ebfe7bd2f1..73244c938b10 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -234,6 +234,11 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, 
uint32_t code)
 goto out_write;
 }
 
+if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + 
PAGE_SIZE)) {
+work_sccb.h.response_code = 
cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+goto out_write;
+}
+
 sclp_c->execute(sclp, &work_sccb, code);
 out_write:
 cpu_physical_memory_write(sccb, &work_sccb,
-- 
2.21.0

[PULL 00/12] s390x qemu updates 20190930

[Christian Borntraeger]

Peter,

The following changes since commit 786d36ad416c6c199b18b78cc31eddfb784fe15d:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20190927' 
into staging (2019-09-30 11:02:22 +0100)

are available in the Git repository at:

  git://github.com/borntraeger/qemu.git tags/s390x-20190930

for you to fetch changes up to c5b9ce518c0551d0198bcddadc82e03de9ac8de9:

  s390/kvm: split kvm mem slots at 4TB (2019-09-30 13:51:50 +0200)


- do not abuse memory_region_allocate_system_memory and split the memory
  according to KVM memslots in KVM code instead (Paolo, Igor)
- change splitting to split at 4TB (Christian)
- do not claim s390 (31bit) support in configure (Thomas)
- sclp error checking (Janosch, Claudio)
- new s390 pci maintainer (Matt, Collin)
- fix s390 pci (again) (Matt)


Christian Borntraeger (1):
  s390/kvm: split kvm mem slots at 4TB

Claudio Imbrenda (1):
  s390x: sclp: Report insufficient SCCB length

Igor Mammedov (2):
  kvm: split too big memory section on several memslots
  s390: do not call memory_region_allocate_system_memory() multiple times

Janosch Frank (3):
  s390x: sclp: refactor invalid command check
  s390x: sclp: boundary check
  s390x: sclp: fix error handling for oversize control blocks

Matthew Rosato (2):
  MAINTAINERS: Update S390 PCI Maintainer
  s390: PCI: fix IOMMU region init

Paolo Bonzini (2):
  kvm: extract kvm_log_clear_one_slot
  kvm: clear dirty bitmaps from all overlapping memslots

Thomas Huth (1):
  configure: Remove s390 (31-bit mode) from the list of supported CPUs

 MAINTAINERS|   2 +-
 accel/kvm/kvm-all.c| 237 -
 configure  |   2 +-
 hw/s390x/event-facility.c  |   3 -
 hw/s390x/s390-pci-bus.c|   7 +-
 hw/s390x/s390-virtio-ccw.c |  30 +-
 hw/s390x/sclp.c|  37 ++-
 include/sysemu/kvm_int.h   |   1 +
 target/s390x/kvm.c |  10 ++
 9 files changed, 202 insertions(+), 127 deletions(-)

[PULL 06/12] s390x: sclp: Report insufficient SCCB length

[Christian Borntraeger]

From: Claudio Imbrenda 

Return the correct error code when the SCCB buffer is too small to
contain all of the output, for the Read SCP Information and
Read CPU Information commands.

Signed-off-by: Claudio Imbrenda 
Reviewed-by: Jason J. Herne 
Message-Id: <1569591203-15258-5-git-send-email-imbre...@linux.ibm.com>
Reviewed-by: David Hildenbrand 
Signed-off-by: Christian Borntraeger 
---
 hw/s390x/sclp.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index abb6e5011f9c..f57ce7b73943 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -68,6 +68,12 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 
 read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
 
+if (be16_to_cpu(sccb->h.length) <
+(sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
+sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+return;
+}
+
 /* Configuration Characteristic (Extension) */
 s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR,
  read_info->conf_char);
@@ -118,6 +124,12 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB 
*sccb)
 cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
 cpu_info->nr_standby = cpu_to_be16(0);
 
+if (be16_to_cpu(sccb->h.length) <
+(sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
+sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+return;
+}
+
 /* The standby offset is 16-byte for each CPU */
 cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
 + cpu_info->nr_configured*sizeof(CPUEntry));
-- 
2.21.0

[PULL 08/12] kvm: extract kvm_log_clear_one_slot

[Christian Borntraeger]

From: Paolo Bonzini 

We may need to clear the dirty bitmap for more than one KVM memslot.
First do some code movement with no semantic change.

Signed-off-by: Paolo Bonzini 
Signed-off-by: Igor Mammedov 
Reviewed-by: Peter Xu 
Message-Id: <20190924144751.24149-2-imamm...@redhat.com>
Acked-by: Paolo Bonzini 
Signed-off-by: Christian Borntraeger 
[fixup line break]
---
 accel/kvm/kvm-all.c | 103 
 1 file changed, 57 insertions(+), 46 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b09bad08048d..a85ec09486dd 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -575,55 +575,14 @@ out:
 #define KVM_CLEAR_LOG_ALIGN  (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
 #define KVM_CLEAR_LOG_MASK   (-KVM_CLEAR_LOG_ALIGN)
 
-/**
- * kvm_physical_log_clear - Clear the kernel's dirty bitmap for range
- *
- * NOTE: this will be a no-op if we haven't enabled manual dirty log
- * protection in the host kernel because in that case this operation
- * will be done within log_sync().
- *
- * @kml: the kvm memory listener
- * @section: the memory range to clear dirty bitmap
- */
-static int kvm_physical_log_clear(KVMMemoryListener *kml,
-  MemoryRegionSection *section)
+static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
+  uint64_t size)
 {
 KVMState *s = kvm_state;
+uint64_t end, bmap_start, start_delta, bmap_npages;
 struct kvm_clear_dirty_log d;
-uint64_t start, end, bmap_start, start_delta, bmap_npages, size;
 unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
-KVMSlot *mem = NULL;
-int ret, i;
-
-if (!s->manual_dirty_log_protect) {
-/* No need to do explicit clear */
-return 0;
-}
-
-start = section->offset_within_address_space;
-size = int128_get64(section->size);
-
-if (!size) {
-/* Nothing more we can do... */
-return 0;
-}
-
-kvm_slots_lock(kml);
-
-/* Find any possible slot that covers the section */
-for (i = 0; i < s->nr_slots; i++) {
-mem = &kml->slots[i];
-if (mem->start_addr <= start &&
-start + size <= mem->start_addr + mem->memory_size) {
-break;
-}
-}
-
-/*
- * We should always find one memslot until this point, otherwise
- * there could be something wrong from the upper layer
- */
-assert(mem && i != s->nr_slots);
+int ret;
 
 /*
  * We need to extend either the start or the size or both to
@@ -694,7 +653,7 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
 /* It should never overflow.  If it happens, say something */
 assert(bmap_npages <= UINT32_MAX);
 d.num_pages = bmap_npages;
-d.slot = mem->slot | (kml->as_id << 16);
+d.slot = mem->slot | (as_id << 16);
 
 if (kvm_vm_ioctl(s, KVM_CLEAR_DIRTY_LOG, &d) == -1) {
 ret = -errno;
@@ -717,6 +676,58 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
  size / psize);
 /* This handles the NULL case well */
 g_free(bmap_clear);
+return ret;
+}
+
+
+/**
+ * kvm_physical_log_clear - Clear the kernel's dirty bitmap for range
+ *
+ * NOTE: this will be a no-op if we haven't enabled manual dirty log
+ * protection in the host kernel because in that case this operation
+ * will be done within log_sync().
+ *
+ * @kml: the kvm memory listener
+ * @section: the memory range to clear dirty bitmap
+ */
+static int kvm_physical_log_clear(KVMMemoryListener *kml,
+  MemoryRegionSection *section)
+{
+KVMState *s = kvm_state;
+uint64_t start, size;
+KVMSlot *mem = NULL;
+int ret, i;
+
+if (!s->manual_dirty_log_protect) {
+/* No need to do explicit clear */
+return 0;
+}
+
+start = section->offset_within_address_space;
+size = int128_get64(section->size);
+
+if (!size) {
+/* Nothing more we can do... */
+return 0;
+}
+
+kvm_slots_lock(kml);
+
+/* Find any possible slot that covers the section */
+for (i = 0; i < s->nr_slots; i++) {
+mem = &kml->slots[i];
+if (mem->start_addr <= start &&
+start + size <= mem->start_addr + mem->memory_size) {
+break;
+}
+}
+
+/*
+ * We should always find one memslot until this point, otherwise
+ * there could be something wrong from the upper layer
+ */
+assert(mem && i != s->nr_slots);
+ret = kvm_log_clear_one_slot(mem, kml->as_id, start, size);
 
 kvm_slots_unlock(kml);
 
-- 
2.21.0

[PULL 03/12] s390x: sclp: refactor invalid command check

[Christian Borntraeger]

From: Janosch Frank 

Invalid command checking has to be done before the boundary check,
refactoring it now allows to insert the boundary check at the correct
place later.

Signed-off-by: Janosch Frank 
Reviewed-by: Jason J. Herne 
Message-Id: <1569591203-15258-2-git-send-email-imbre...@linux.ibm.com>
Reviewed-by: David Hildenbrand 
Signed-off-by: Christian Borntraeger 
---
 hw/s390x/event-facility.c |  3 ---
 hw/s390x/sclp.c   | 17 -
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 797ecbb7a9c8..66205697ae75 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -377,9 +377,6 @@ static void command_handler(SCLPEventFacility *ef, SCCB 
*sccb, uint64_t code)
 case SCLP_CMD_WRITE_EVENT_MASK:
 write_event_mask(ef, sccb);
 break;
-default:
-sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
-break;
 }
 }
 
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index fac7c3bb6c02..95ebfe7bd2f1 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -219,8 +219,23 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, 
uint32_t code)
 goto out;
 }
 
-sclp_c->execute(sclp, &work_sccb, code);
+switch (code & SCLP_CMD_CODE_MASK) {
+case SCLP_CMDW_READ_SCP_INFO:
+case SCLP_CMDW_READ_SCP_INFO_FORCED:
+case SCLP_CMDW_READ_CPU_INFO:
+case SCLP_CMDW_CONFIGURE_IOA:
+case SCLP_CMDW_DECONFIGURE_IOA:
+case SCLP_CMD_READ_EVENT_DATA:
+case SCLP_CMD_WRITE_EVENT_DATA:
+case SCLP_CMD_WRITE_EVENT_MASK:
+break;
+default:
+work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+goto out_write;
+}
 
+sclp_c->execute(sclp, &work_sccb, code);
+out_write:
 cpu_physical_memory_write(sccb, &work_sccb,
   be16_to_cpu(work_sccb.h.length));
 
-- 
2.21.0

[PULL 02/12] s390: PCI: fix IOMMU region init

[Christian Borntraeger]

From: Matthew Rosato 

The fix in dbe9cf606c shrinks the IOMMU memory region to a size
that seems reasonable on the surface, however is actually too
small as it is based against a 0-mapped address space.  This
causes breakage with small guests as they can overrun the IOMMU window.

Let's go back to the prior method of initializing iommu for now.

Fixes: dbe9cf606c ("s390x/pci: Set the iommu region size mpcifc request")
Cc: qemu-sta...@nongnu.org
Reviewed-by: Pierre Morel 
Reported-by: Boris Fiuczynski 
Tested-by: Boris Fiuczynski 
Reported-by: Stefan Zimmerman 
Signed-off-by: Matthew Rosato 
Message-Id: <1569507036-15314-1-git-send-email-mjros...@linux.ibm.com>
Signed-off-by: Christian Borntraeger 
---
 hw/s390x/s390-pci-bus.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 963a41c7f532..2d2f4a7c419c 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -695,10 +695,15 @@ static const MemoryRegionOps s390_msi_ctrl_ops = {
 
 void s390_pci_iommu_enable(S390PCIIOMMU *iommu)
 {
+/*
+ * The iommu region is initialized against a 0-mapped address space,
+ * so the smallest IOMMU region we can define runs from 0 to the end
+ * of the PCI address space.
+ */
 char *name = g_strdup_printf("iommu-s390-%04x", iommu->pbdev->uid);
 memory_region_init_iommu(&iommu->iommu_mr, sizeof(iommu->iommu_mr),
  TYPE_S390_IOMMU_MEMORY_REGION, OBJECT(&iommu->mr),
- name, iommu->pal - iommu->pba + 1);
+ name, iommu->pal + 1);
 iommu->enabled = true;
 memory_region_add_subregion(&iommu->mr, 0, 
MEMORY_REGION(&iommu->iommu_mr));
 g_free(name);
-- 
2.21.0

Re: Arch info lost in "info cpus"

[Viktor Mihajlovski]

On 9/30/19 10:45 AM, Dr. David Alan Gilbert wrote:
> * Sergio Lopez (s...@redhat.com) wrote:
>> Hi,
>>
>> Commit 137b5cb6ab565cb3781d5337591e155932b4230e (hmp: change
>> hmp_info_cpus to use query-cpus-fast) updated the "info cpus" commit to
>> make it more lightweight, but also removed the ability to get the
>> architecture specific status of each vCPU.
>>
>> This information was really useful to diagnose certain Guest issues,
>> without the need of using GDB, which is more intrusive and requires
>> enabling it in advance.
>>
>> Is there an alternative way of getting something equivalent to what
>> "info cpus" provided previously (in 2.10)?
> Even the qemp equivalent, query-cpus is deprecated.
> (Although we do call the underlying query-cpus in 'info numa' as well)
This obviously went by unnoticed back then. Query-cpus-fast should serve
the same purpose as query-cpus there, being less intrusive on the VM and
allow to complete the deprecation process, if this is still wanted. If
not, adding an option that lets hmp 'info cpus' call the old API doesn't
seem unreasonable.

>
> Dave
>
>> Thanks,
>> Sergio.
>
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>
-- 
Kind Regards,
   Viktor

Re: [PATCH v2] Disallow colons in the parameter of "-accel"

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190930123505.11607-1-th...@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/20190930123505.11607-1-th...@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH 15/18] iotests: Make 137 work with data_file

[Max Reitz]

On 29.09.19 18:38, Maxim Levitsky wrote:
> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
>> When using an external data file, there are no refcounts for data
>> clusters.  We thus have to adjust the corruption test in this patch to
>> not be based around a data cluster allocation, but the L2 table
>> allocation (L2 tables are still refcounted with external data files).
>>
>> Doing so means this test works both with and without external data
>> files.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/137 | 10 ++
>>  tests/qemu-iotests/137.out |  4 +---
>>  2 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
>> index 6cf2997577..dd3484205e 100755
>> --- a/tests/qemu-iotests/137
>> +++ b/tests/qemu-iotests/137
>> @@ -138,14 +138,16 @@ $QEMU_IO \
>>  "$TEST_IMG" 2>&1 | _filter_qemu_io
>>  
>>  # The dirty bit must not be set
>> -$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
>> +# (Filter the external data file bit)
>> +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features \
>> +| sed -e 's/0x4/0x0/'
> Maybe it is better to filter all the feature bits, but the dirty bit,
> since only it is needed here, so that when we start running tests with
> more features, we won't need to do this again?

I’d hate a filter s/[02468ace]$/no dirty bit/ though.

>>  
>>  # Similarly we can test whether corruption detection has been enabled:
>> -# Create L1/L2, overwrite first entry in refcount block, allocate something.
>> +# Create L1, overwrite refcounts, force allocation of L2 by writing
>> +# data.
>>  # Disabling the checks should fail, so the corruption must be detected.
>>  _make_test_img 64M
>> -$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
>> -poke_file "$TEST_IMG" "$((0x2))" "\x00\x00"
>> +poke_file "$TEST_IMG" "$((0x2))" "\x00\x00\x00\x00\x00\x00\x00\x00"
> 
> I am wondering if there is any better way to do this (regardless of this 
> patch),
> Basically the above code pokes into the 3rd cluster on the disk I *think*, 
> and I don't
> understand why it has to contain refcounts. Hmm...
> First cluster I can guess will have the header, 2nd cluster probably L1 
> table, and 3rd, refcounts?
> If so, the test should specify that it needs 64K clusters, because the day 
> will come when
> we will need to test this as well, but I guess in a separate patch,

When that day comes, a whole lot of other stuff will break, too.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 14/18] iotests: Make 110 work with data_file

[Maxim Levitsky]

On Mon, 2019-09-30 at 15:34 +0200, Max Reitz wrote:
> On 29.09.19 18:34, Maxim Levitsky wrote:
> > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
> > > The only difference is that the json:{} filename of the image looks
> > > different.  We actually do not care about that filename in this test, we
> > > are only interested in (1) that there is a json:{} filename, and (2)
> > > whether the backing filename can be constructed.
> > > 
> > > So just filter out the json:{} data, thus making this test pass both
> > > with and without data_file.
> > > 
> > > Signed-off-by: Max Reitz 
> > > ---
> > >  tests/qemu-iotests/110 | 7 +--
> > >  tests/qemu-iotests/110.out | 4 ++--
> > >  2 files changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
> > > index f78df0e6e1..34459dcd60 100755
> > > --- a/tests/qemu-iotests/110
> > > +++ b/tests/qemu-iotests/110
> > > @@ -67,6 +67,7 @@ echo
> > >  # Across blkdebug without a config file, you cannot reconstruct 
> > > filenames, so
> > >  # qemu is incapable of knowing the directory of the top image from the 
> > > filename
> > >  # alone. However, using bdrv_dirname(), it should still work.
> > > +# (Filter out the json:{} filename so this test works with external data 
> > > files)
> > >  TEST_IMG="json:{
> > >  'driver': '$IMGFMT',
> > >  'file': {
> > > @@ -82,7 +83,8 @@ TEST_IMG="json:{
> > >  }
> > >  ]
> > >  }
> > > -}" _img_info | _filter_img_info | grep -v 'backing file format'
> > > +}" _img_info | _filter_img_info | grep -v 'backing file format' \
> > > +| sed -e 's#^image: json:.*#image: json:{ /* filtered */ }#'
> > >  
> > >  echo
> > >  echo '=== Backing name is always relative to the backed image ==='
> > > @@ -114,7 +116,8 @@ TEST_IMG="json:{
> > >  }
> > >  ]
> > >  }
> > > -}" _img_info | _filter_img_info | grep -v 'backing file format'
> > > +}" _img_info | _filter_img_info | grep -v 'backing file format' \
> > > +| sed -e 's#^image: json:.*#image: json:{ /* filtered */ }#'
> > >  
> > >  
> > >  # success, all done
> > > diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
> > > index f60b26390e..f835553a99 100644
> > > --- a/tests/qemu-iotests/110.out
> > > +++ b/tests/qemu-iotests/110.out
> > > @@ -11,7 +11,7 @@ backing file: t.IMGFMT.base (actual path: 
> > > TEST_DIR/t.IMGFMT.base)
> > >  
> > >  === Non-reconstructable filename ===
> > >  
> > > -image: json:{"driver": "IMGFMT", "file": {"set-state.0.event": 
> > > "read_aio", "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, 
> > > "driver": "blkdebug", "set-state.0.new_state": 42}}
> > > +image: json:{ /* filtered */ }
> > >  file format: IMGFMT
> > >  virtual size: 64 MiB (67108864 bytes)
> > >  backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
> > > @@ -22,7 +22,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT 
> > > size=67108864 backing_file=t.IMGFMT.b
> > >  
> > >  === Nodes without a common directory ===
> > >  
> > > -image: json:{"driver": "IMGFMT", "file": {"children": [{"driver": 
> > > "file", "filename": "TEST_DIR/t.IMGFMT"}, {"driver": "file", "filename": 
> > > "TEST_DIR/t.IMGFMT.copy"}], "driver": "quorum", "vote-
> > > threshold": 1}}
> > > +image: json:{ /* filtered */ }
> > >  file format: IMGFMT
> > >  virtual size: 64 MiB (67108864 bytes)
> > >  backing file: t.IMGFMT.base (cannot determine actual path)
> > 
> > Again, maybe put that into the common.filter, so new tests won't need to 
> > copy&paste this?
> 
> Good idea.
> 
> > Also maybe remove the image name completely from output, thus not needing 
> > the more complex regex?
> 
> I’d prefer to still see that there is a json:{} filename instead of a
> plain one.  (This is important in this test because for plain filenames,
> it’s generally easy to reconstruct the backing file path; it’s only
> difficult for json:{} filenames.)

All right, seems good.


Best regards,
Maxim Levitsky
> 
> Max
> 

Re: [PATCH 14/18] iotests: Make 110 work with data_file

[Max Reitz]

On 29.09.19 18:34, Maxim Levitsky wrote:
> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
>> The only difference is that the json:{} filename of the image looks
>> different.  We actually do not care about that filename in this test, we
>> are only interested in (1) that there is a json:{} filename, and (2)
>> whether the backing filename can be constructed.
>>
>> So just filter out the json:{} data, thus making this test pass both
>> with and without data_file.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/110 | 7 +--
>>  tests/qemu-iotests/110.out | 4 ++--
>>  2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
>> index f78df0e6e1..34459dcd60 100755
>> --- a/tests/qemu-iotests/110
>> +++ b/tests/qemu-iotests/110
>> @@ -67,6 +67,7 @@ echo
>>  # Across blkdebug without a config file, you cannot reconstruct filenames, 
>> so
>>  # qemu is incapable of knowing the directory of the top image from the 
>> filename
>>  # alone. However, using bdrv_dirname(), it should still work.
>> +# (Filter out the json:{} filename so this test works with external data 
>> files)
>>  TEST_IMG="json:{
>>  'driver': '$IMGFMT',
>>  'file': {
>> @@ -82,7 +83,8 @@ TEST_IMG="json:{
>>  }
>>  ]
>>  }
>> -}" _img_info | _filter_img_info | grep -v 'backing file format'
>> +}" _img_info | _filter_img_info | grep -v 'backing file format' \
>> +| sed -e 's#^image: json:.*#image: json:{ /* filtered */ }#'
>>  
>>  echo
>>  echo '=== Backing name is always relative to the backed image ==='
>> @@ -114,7 +116,8 @@ TEST_IMG="json:{
>>  }
>>  ]
>>  }
>> -}" _img_info | _filter_img_info | grep -v 'backing file format'
>> +}" _img_info | _filter_img_info | grep -v 'backing file format' \
>> +| sed -e 's#^image: json:.*#image: json:{ /* filtered */ }#'
>>  
>>  
>>  # success, all done
>> diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
>> index f60b26390e..f835553a99 100644
>> --- a/tests/qemu-iotests/110.out
>> +++ b/tests/qemu-iotests/110.out
>> @@ -11,7 +11,7 @@ backing file: t.IMGFMT.base (actual path: 
>> TEST_DIR/t.IMGFMT.base)
>>  
>>  === Non-reconstructable filename ===
>>  
>> -image: json:{"driver": "IMGFMT", "file": {"set-state.0.event": "read_aio", 
>> "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": 
>> "blkdebug", "set-state.0.new_state": 42}}
>> +image: json:{ /* filtered */ }
>>  file format: IMGFMT
>>  virtual size: 64 MiB (67108864 bytes)
>>  backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
>> @@ -22,7 +22,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
>> backing_file=t.IMGFMT.b
>>  
>>  === Nodes without a common directory ===
>>  
>> -image: json:{"driver": "IMGFMT", "file": {"children": [{"driver": "file", 
>> "filename": "TEST_DIR/t.IMGFMT"}, {"driver": "file", "filename": 
>> "TEST_DIR/t.IMGFMT.copy"}], "driver": "quorum", "vote-
>> threshold": 1}}
>> +image: json:{ /* filtered */ }
>>  file format: IMGFMT
>>  virtual size: 64 MiB (67108864 bytes)
>>  backing file: t.IMGFMT.base (cannot determine actual path)
> 
> Again, maybe put that into the common.filter, so new tests won't need to 
> copy&paste this?

Good idea.

> Also maybe remove the image name completely from output, thus not needing the 
> more complex regex?

I’d prefer to still see that there is a json:{} filename instead of a
plain one.  (This is important in this test because for plain filenames,
it’s generally easy to reconstruct the backing file path; it’s only
difficult for json:{} filenames.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 01/18] iotests: Filter refcount_order in 036

[Maxim Levitsky]

On Mon, 2019-09-30 at 14:43 +0200, Max Reitz wrote:
> On 29.09.19 18:31, Maxim Levitsky wrote:
> > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
> > > This test can run just fine with other values for refcount_bits, so we
> > > should filter the value from qcow2.py's dump-header.
> > > 
> > > (036 currently ignores user-specified image options, but that will be
> > > fixed in the next patch.)
> > > 
> > > Signed-off-by: Max Reitz 
> > > ---
> > >  tests/qemu-iotests/036 | 9 ++---
> > >  tests/qemu-iotests/036.out | 6 +++---
> > >  2 files changed, 9 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
> > > index f06ff67408..69d0f9f903 100755
> > > --- a/tests/qemu-iotests/036
> > > +++ b/tests/qemu-iotests/036
> > > @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit 
> > > incompatible 63
> > >  
> > >  # Without feature table
> > >  $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857
> > > -$PYTHON qcow2.py "$TEST_IMG" dump-header
> > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \
> > > +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
> > >  _img_info
> > >  
> > >  # With feature table containing bit 63
> > > @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear 
> > > feature bit ===
> > >  echo
> > >  _make_test_img 64M
> > >  $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63
> > > -$PYTHON qcow2.py "$TEST_IMG" dump-header
> > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \
> > > +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
> > >  
> > >  echo
> > >  echo === Repair image ===
> > >  echo
> > >  _check_test_img -r all
> > >  
> > > -$PYTHON qcow2.py "$TEST_IMG" dump-header
> > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \
> > > +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
> > >  
> > >  # success, all done
> > >  echo "*** done"
> > > diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
> > > index e489b44386..998c2a8a35 100644
> > > --- a/tests/qemu-iotests/036.out
> > > +++ b/tests/qemu-iotests/036.out
> > > @@ -19,7 +19,7 @@ snapshot_offset   0x0
> > >  incompatible_features 0x8000
> > >  compatible_features   0x0
> > >  autoclear_features0x0
> > > -refcount_order4
> > > +refcount_order(filtered)
> > >  header_length 104
> > >  
> > >  qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT 
> > > feature(s): Unknown incompatible feature: 8000
> > > @@ -53,7 +53,7 @@ snapshot_offset   0x0
> > >  incompatible_features 0x0
> > >  compatible_features   0x0
> > >  autoclear_features0x8000
> > > -refcount_order4
> > > +refcount_order(filtered)
> > >  header_length 104
> > >  
> > >  Header extension:
> > > @@ -81,7 +81,7 @@ snapshot_offset   0x0
> > >  incompatible_features 0x0
> > >  compatible_features   0x0
> > >  autoclear_features0x0
> > > -refcount_order4
> > > +refcount_order(filtered)
> > >  header_length 104
> > >  
> > >  Header extension:
> > 
> > Minor notes:
> > 
> > 1. Maybe put the sed command into a function to avoid duplication?
> 
> Hm, maybe, but that would increase the LoC, so I’m not sure whether it
> really would be a simplification.
IMHO, in my opinion, regardless of LOC, duplicated code is almost always
bad. Common functions are mostly for the next guy that will be able to use
them instead of searching through code to see how this is done there and there.

> 
> > 2. As I understand the test, it only checks the feature bits.
> >So maybe instead of blacklisting some of the values, white list
> >only the feature bits in the output?
> 
> Hm.  Seems reasonable, but then again I’d prefer a minimal change, and
> changing it to a whitelist would be more change...
I don't think this is bad, again in my eyes, the code should be as friendly
as possible for the next guy that will have to change it, so adding
some more extra changes don't seem a problem to me.
Of course this is only my personal option and each approach has its own cons,
and pros. This doesn't matter that much to me.

Best regards,
Maxim Levitsky

Re: [PATCH 18/18] iotests: Allow check -o data_file

[Max Reitz]

On 29.09.19 18:39, Maxim Levitsky wrote:
> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
>> The problem with allowing the data_file option is that you want to use a
>> different data file per image used in the test.  Therefore, we need to
>> allow patterns like -o data_file='$TEST_IMG.data_file'.
>>
>> Then, we need to filter it out from qemu-img map, qemu-img create, and
>> remove the data file in _rm_test_img.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/common.filter | 21 +++--
>>  tests/qemu-iotests/common.rc | 10 +-
>>  2 files changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/common.filter 
>> b/tests/qemu-iotests/common.filter
>> index 841f7642af..a11f9a8972 100644
>> --- a/tests/qemu-iotests/common.filter
>> +++ b/tests/qemu-iotests/common.filter
>> @@ -115,7 +115,15 @@ _filter_actual_image_size()
>>  # replace driver-specific options in the "Formatting..." line
>>  _filter_img_create()
>>  {
>> -$SED -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
>> +data_file_filter=()
>> +if echo "$IMGOPTS" | grep -q 'data_file='; then
>> +data_file=$(echo "$IMGOPTS" | sed -e 
>> 's/.*\(data_file=[^,]*\).*/\1/' \
>> +  -e "s#\\\$TEST_IMG#$TEST_IMG#")
> Could you maybe add a comment on this regular expression, similar
> to what is in the commit message?
> 
>> +data_file_filter=(-e "s# $data_file##")
> Why to use array here? A variable would work too I think.

Because I find it to be much simpler to quote correctly.

>> +fi
>> +
>> +$SED "${data_file_filter[@]}" \
>> +-e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
>>  -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
>>  -e "s#$TEST_DIR#TEST_DIR#g" \
>>  -e "s#$IMGFMT#IMGFMT#g" \
>> @@ -198,9 +206,18 @@ _filter_img_info()
>>  # human and json output
>>  _filter_qemu_img_map()
>>  {
>> +data_file_filter=()
>> +if echo "$IMGOPTS" | grep -q 'data_file='; then
>> +data_file_pattern=$(echo "$IMGOPTS" | sed -e 
>> 's/.*data_file=\([^,]*\).*/\1/' \
>> +  -e 
>> 's#\$TEST_IMG#\\(.*\\)#')
>> +data_file_filter=(-e "s#$data_file_pattern#\\1#")
> Seems to do the same thing, but in slightly different way.

It does a very different thing, in that it creates a sed pattern.

I’d first need to get comfortable with the thought of having a common
function _data_file_from_imgopts and then pass a '\(.*\)' to it, which
would look weird.

Or maybe even a '\\(.*\\)', because I’d still have to take the sed
escaping into account, even in that function’s caller, which is weird, too.

> Maybe make a function to avoid duplication?
> I am not 100% sure though.
> 
>> +fi
>> +
>>  $SED -e 's/\([0-9a-fx]* *[0-9a-fx]* *\)[0-9a-fx]* */\1/g' \
>>  -e 's/"offset": [0-9]\+/"offset": OFFSET/g' \
>> --e 's/Mapped to *//' | _filter_testdir | _filter_imgfmt
>> +-e 's/Mapped to *//' \
>> +"${data_file_filter[@]}" \
>> +| _filter_testdir | _filter_imgfmt
>>  }
>>  
>>  _filter_nbd()
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index f3784077de..834ece12e4 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -297,7 +297,8 @@ _make_test_img()
>>  fi
>>  
>>  if [ -n "$IMGOPTS" ]; then
>> -optstr=$(_optstr_add "$optstr" "$IMGOPTS")
>> +imgopts_expanded=$(echo "$IMGOPTS" | sed -e 
>> "s#\\\$TEST_IMG#$img_name#")
>> +optstr=$(_optstr_add "$optstr" "$imgopts_expanded")
> Also a comment explaining why this is needed would be welcome.
> 
>>  fi
>>  if [ -n "$IMGKEYSECRET" ]; then
>>  object_options="--object secret,id=keysec0,data=$IMGKEYSECRET"
>> @@ -376,6 +377,13 @@ _rm_test_img()
>>  # Remove all the extents for vmdk
>>  "$QEMU_IMG" info "$img" 2>/dev/null | grep 'filename:' | cut -f 2 
>> -d: \
>>  | xargs -I {} rm -f "{}"
>> +elif [ "$IMGFMT" = "qcow2" ]; then
>> +# Remove external data file
>> +if echo "$IMGOPTS" | grep -q 'data_file='; then
>> +data_file=$(echo "$IMGOPTS" | sed -e 
>> 's/.*data_file=\([^,]*\).*/\1/' \
>> +  -e "s#\\\$TEST_IMG#$img#")
> Same here, even more evidence that it would be nice to move this to a common 
> function.

I’m not sure whether it’s more evidence because it would increase LoC.
I may or may not do it.

Max

> Again, I am not 100% sure that this is the same regex, but it looks like that.
> 
>> +rm -f "$data_file"
>> +fi
>>  fi
>>  rm -f "$img"
>>  }
> 
> 
> Best regards,
>   Maxim Levitsky
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 01/18] iotests: Filter refcount_order in 036

[Max Reitz]

On 30.09.19 15:40, Maxim Levitsky wrote:
> On Mon, 2019-09-30 at 14:43 +0200, Max Reitz wrote:
>> On 29.09.19 18:31, Maxim Levitsky wrote:
>>> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
 This test can run just fine with other values for refcount_bits, so we
 should filter the value from qcow2.py's dump-header.

 (036 currently ignores user-specified image options, but that will be
 fixed in the next patch.)

 Signed-off-by: Max Reitz 
 ---
  tests/qemu-iotests/036 | 9 ++---
  tests/qemu-iotests/036.out | 6 +++---
  2 files changed, 9 insertions(+), 6 deletions(-)

 diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
 index f06ff67408..69d0f9f903 100755
 --- a/tests/qemu-iotests/036
 +++ b/tests/qemu-iotests/036
 @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit 
 incompatible 63
  
  # Without feature table
  $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857
 -$PYTHON qcow2.py "$TEST_IMG" dump-header
 +$PYTHON qcow2.py "$TEST_IMG" dump-header \
 +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
  _img_info
  
  # With feature table containing bit 63
 @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear feature 
 bit ===
  echo
  _make_test_img 64M
  $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63
 -$PYTHON qcow2.py "$TEST_IMG" dump-header
 +$PYTHON qcow2.py "$TEST_IMG" dump-header \
 +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
  
  echo
  echo === Repair image ===
  echo
  _check_test_img -r all
  
 -$PYTHON qcow2.py "$TEST_IMG" dump-header
 +$PYTHON qcow2.py "$TEST_IMG" dump-header \
 +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
  
  # success, all done
  echo "*** done"
 diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
 index e489b44386..998c2a8a35 100644
 --- a/tests/qemu-iotests/036.out
 +++ b/tests/qemu-iotests/036.out
 @@ -19,7 +19,7 @@ snapshot_offset   0x0
  incompatible_features 0x8000
  compatible_features   0x0
  autoclear_features0x0
 -refcount_order4
 +refcount_order(filtered)
  header_length 104
  
  qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT 
 feature(s): Unknown incompatible feature: 8000
 @@ -53,7 +53,7 @@ snapshot_offset   0x0
  incompatible_features 0x0
  compatible_features   0x0
  autoclear_features0x8000
 -refcount_order4
 +refcount_order(filtered)
  header_length 104
  
  Header extension:
 @@ -81,7 +81,7 @@ snapshot_offset   0x0
  incompatible_features 0x0
  compatible_features   0x0
  autoclear_features0x0
 -refcount_order4
 +refcount_order(filtered)
  header_length 104
  
  Header extension:
>>>
>>> Minor notes:
>>>
>>> 1. Maybe put the sed command into a function to avoid duplication?
>>
>> Hm, maybe, but that would increase the LoC, so I’m not sure whether it
>> really would be a simplification.
> IMHO, in my opinion, regardless of LOC, duplicated code is almost always
> bad. Common functions are mostly for the next guy that will be able to use
> them instead of searching through code to see how this is done there and 
> there.

In my experience, people write tests without scanning common.filter for
whether anyone has written the same code already.  See the
_filter_qemu_img_check example in this series.

>>
>>> 2. As I understand the test, it only checks the feature bits.
>>>So maybe instead of blacklisting some of the values, white list
>>>only the feature bits in the output?
>>
>> Hm.  Seems reasonable, but then again I’d prefer a minimal change, and
>> changing it to a whitelist would be more change...
> I don't think this is bad, again in my eyes, the code should be as friendly
> as possible for the next guy that will have to change it, so adding
> some more extra changes don't seem a problem to me.

In my eyes tests aren’t code.

Max

> Of course this is only my personal option and each approach has its own cons,
> and pros. This doesn't matter that much to me.
> 
> Best regards,
>   Maxim Levitsky
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 15/18] iotests: Make 137 work with data_file

[Maxim Levitsky]

On Mon, 2019-09-30 at 15:38 +0200, Max Reitz wrote:
> On 29.09.19 18:38, Maxim Levitsky wrote:
> > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
> > > When using an external data file, there are no refcounts for data
> > > clusters.  We thus have to adjust the corruption test in this patch to
> > > not be based around a data cluster allocation, but the L2 table
> > > allocation (L2 tables are still refcounted with external data files).
> > > 
> > > Doing so means this test works both with and without external data
> > > files.
> > > 
> > > Signed-off-by: Max Reitz 
> > > ---
> > >  tests/qemu-iotests/137 | 10 ++
> > >  tests/qemu-iotests/137.out |  4 +---
> > >  2 files changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
> > > index 6cf2997577..dd3484205e 100755
> > > --- a/tests/qemu-iotests/137
> > > +++ b/tests/qemu-iotests/137
> > > @@ -138,14 +138,16 @@ $QEMU_IO \
> > >  "$TEST_IMG" 2>&1 | _filter_qemu_io
> > >  
> > >  # The dirty bit must not be set
> > > -$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
> > > +# (Filter the external data file bit)
> > > +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features \
> > > +| sed -e 's/0x4/0x0/'
> > 
> > Maybe it is better to filter all the feature bits, but the dirty bit,
> > since only it is needed here, so that when we start running tests with
> > more features, we won't need to do this again?
> 
> I’d hate a filter s/[02468ace]$/no dirty bit/ though.
Nothing a helper function can't solve IMHO, I would convert this to a number,
and then check bitwise the bit 2, assuming that is the dirty bit)
Again, note that my approach to code is to make it as easy as possible for the
next guy to change, so I am noticing such places. Eventually someone of us,
will be that next guy. Then again, I don't mind leaving this as is, just noting 
this.
> 
> > >  
> > >  # Similarly we can test whether corruption detection has been enabled:
> > > -# Create L1/L2, overwrite first entry in refcount block, allocate 
> > > something.
> > > +# Create L1, overwrite refcounts, force allocation of L2 by writing
> > > +# data.
> > >  # Disabling the checks should fail, so the corruption must be detected.
> > >  _make_test_img 64M
> > > -$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
> > > -poke_file "$TEST_IMG" "$((0x2))" "\x00\x00"
> > > +poke_file "$TEST_IMG" "$((0x2))" "\x00\x00\x00\x00\x00\x00\x00\x00"
> > 
> > I am wondering if there is any better way to do this (regardless of this 
> > patch),
> > Basically the above code pokes into the 3rd cluster on the disk I *think*, 
> > and I don't
> > understand why it has to contain refcounts. Hmm...
> > First cluster I can guess will have the header, 2nd cluster probably L1 
> > table, and 3rd, refcounts?
> > If so, the test should specify that it needs 64K clusters, because the day 
> > will come when
> > we will need to test this as well, but I guess in a separate patch,
> 
> When that day comes, a whole lot of other stuff will break, too.
I guess so, won't argue about this one.

Best regards,
Maxim Levitsky


> 
> Max
> 

Re: [PATCH 15/18] iotests: Make 137 work with data_file

[Max Reitz]

On 30.09.19 15:46, Maxim Levitsky wrote:
> On Mon, 2019-09-30 at 15:38 +0200, Max Reitz wrote:
>> On 29.09.19 18:38, Maxim Levitsky wrote:
>>> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
 When using an external data file, there are no refcounts for data
 clusters.  We thus have to adjust the corruption test in this patch to
 not be based around a data cluster allocation, but the L2 table
 allocation (L2 tables are still refcounted with external data files).

 Doing so means this test works both with and without external data
 files.

 Signed-off-by: Max Reitz 
 ---
  tests/qemu-iotests/137 | 10 ++
  tests/qemu-iotests/137.out |  4 +---
  2 files changed, 7 insertions(+), 7 deletions(-)

 diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
 index 6cf2997577..dd3484205e 100755
 --- a/tests/qemu-iotests/137
 +++ b/tests/qemu-iotests/137
 @@ -138,14 +138,16 @@ $QEMU_IO \
  "$TEST_IMG" 2>&1 | _filter_qemu_io
  
  # The dirty bit must not be set
 -$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
 +# (Filter the external data file bit)
 +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features \
 +| sed -e 's/0x4/0x0/'
>>>
>>> Maybe it is better to filter all the feature bits, but the dirty bit,
>>> since only it is needed here, so that when we start running tests with
>>> more features, we won't need to do this again?
>>
>> I’d hate a filter s/[02468ace]$/no dirty bit/ though.
> Nothing a helper function can't solve IMHO, I would convert this to a number,
> and then check bitwise the bit 2, assuming that is the dirty bit)
> Again, note that my approach to code is to make it as easy as possible for the
> next guy to change, so I am noticing such places. Eventually someone of us,
> will be that next guy. Then again, I don't mind leaving this as is, just 
> noting this.

Again, my approach to tests is that they aren’t classical code.

This is a very personal opinion, but I have found that tests that have
the most ad-hoc code with the least function calls are the easiest to
work with.

Tests that have a whole lot of infrastructure and try to have nice code
are a horror to work with because you first have to understand how they
work.

Tests should be simple, not complex.  Some ad-hoc filters do not make
them complex as long as it’s obvious what they do.


Also, the correct approach here is not to do number crunching in bash.
It is to change qcow2.py to emit more easily filterable information.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 01/18] iotests: Filter refcount_order in 036

[Maxim Levitsky]

On Mon, 2019-09-30 at 15:44 +0200, Max Reitz wrote:
> On 30.09.19 15:40, Maxim Levitsky wrote:
> > On Mon, 2019-09-30 at 14:43 +0200, Max Reitz wrote:
> > > On 29.09.19 18:31, Maxim Levitsky wrote:
> > > > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
> > > > > This test can run just fine with other values for refcount_bits, so we
> > > > > should filter the value from qcow2.py's dump-header.
> > > > > 
> > > > > (036 currently ignores user-specified image options, but that will be
> > > > > fixed in the next patch.)
> > > > > 
> > > > > Signed-off-by: Max Reitz 
> > > > > ---
> > > > >  tests/qemu-iotests/036 | 9 ++---
> > > > >  tests/qemu-iotests/036.out | 6 +++---
> > > > >  2 files changed, 9 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
> > > > > index f06ff67408..69d0f9f903 100755
> > > > > --- a/tests/qemu-iotests/036
> > > > > +++ b/tests/qemu-iotests/036
> > > > > @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit 
> > > > > incompatible 63
> > > > >  
> > > > >  # Without feature table
> > > > >  $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857
> > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header
> > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \
> > > > > +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
> > > > >  _img_info
> > > > >  
> > > > >  # With feature table containing bit 63
> > > > > @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear 
> > > > > feature bit ===
> > > > >  echo
> > > > >  _make_test_img 64M
> > > > >  $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63
> > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header
> > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \
> > > > > +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
> > > > >  
> > > > >  echo
> > > > >  echo === Repair image ===
> > > > >  echo
> > > > >  _check_test_img -r all
> > > > >  
> > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header
> > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \
> > > > > +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
> > > > >  
> > > > >  # success, all done
> > > > >  echo "*** done"
> > > > > diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
> > > > > index e489b44386..998c2a8a35 100644
> > > > > --- a/tests/qemu-iotests/036.out
> > > > > +++ b/tests/qemu-iotests/036.out
> > > > > @@ -19,7 +19,7 @@ snapshot_offset   0x0
> > > > >  incompatible_features 0x8000
> > > > >  compatible_features   0x0
> > > > >  autoclear_features0x0
> > > > > -refcount_order4
> > > > > +refcount_order(filtered)
> > > > >  header_length 104
> > > > >  
> > > > >  qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT 
> > > > > feature(s): Unknown incompatible feature: 8000
> > > > > @@ -53,7 +53,7 @@ snapshot_offset   0x0
> > > > >  incompatible_features 0x0
> > > > >  compatible_features   0x0
> > > > >  autoclear_features0x8000
> > > > > -refcount_order4
> > > > > +refcount_order(filtered)
> > > > >  header_length 104
> > > > >  
> > > > >  Header extension:
> > > > > @@ -81,7 +81,7 @@ snapshot_offset   0x0
> > > > >  incompatible_features 0x0
> > > > >  compatible_features   0x0
> > > > >  autoclear_features0x0
> > > > > -refcount_order4
> > > > > +refcount_order(filtered)
> > > > >  header_length 104
> > > > >  
> > > > >  Header extension:
> > > > 
> > > > Minor notes:
> > > > 
> > > > 1. Maybe put the sed command into a function to avoid duplication?
> > > 
> > > Hm, maybe, but that would increase the LoC, so I’m not sure whether it
> > > really would be a simplification.
> > 
> > IMHO, in my opinion, regardless of LOC, duplicated code is almost always
> > bad. Common functions are mostly for the next guy that will be able to use
> > them instead of searching through code to see how this is done there and 
> > there.
> 
> In my experience, people write tests without scanning common.filter for
> whether anyone has written the same code already.  See the
> _filter_qemu_img_check example in this series.
Fair enough, but this can change.

> 
> > > 
> > > > 2. As I understand the test, it only checks the feature bits.
> > > >So maybe instead of blacklisting some of the values, white list
> > > >only the feature bits in the output?
> > > 
> > > Hm.  Seems reasonable, but then again I’d prefer a minimal change, and
> > > changing it to a whitelist would be more change...
> > 
> > I don't think this is bad, again in my eyes, the code should be as friendly
> > as possible for the next guy that will have to change it, so adding
> > some more extra changes don't seem a problem to me.
> 
> In my eyes tests aren’t code.

And yet, writing tests is commonly known as a task 
that developers don't really like to do, so maki

Re: [PATCH 01/18] iotests: Filter refcount_order in 036

[Max Reitz]

On 30.09.19 15:58, Maxim Levitsky wrote:
> On Mon, 2019-09-30 at 15:44 +0200, Max Reitz wrote:
>> On 30.09.19 15:40, Maxim Levitsky wrote:
>>> On Mon, 2019-09-30 at 14:43 +0200, Max Reitz wrote:
 On 29.09.19 18:31, Maxim Levitsky wrote:
> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
>> This test can run just fine with other values for refcount_bits, so we
>> should filter the value from qcow2.py's dump-header.
>>
>> (036 currently ignores user-specified image options, but that will be
>> fixed in the next patch.)
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/036 | 9 ++---
>>  tests/qemu-iotests/036.out | 6 +++---
>>  2 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
>> index f06ff67408..69d0f9f903 100755
>> --- a/tests/qemu-iotests/036
>> +++ b/tests/qemu-iotests/036
>> @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit 
>> incompatible 63
>>  
>>  # Without feature table
>>  $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857
>> -$PYTHON qcow2.py "$TEST_IMG" dump-header
>> +$PYTHON qcow2.py "$TEST_IMG" dump-header \
>> +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
>>  _img_info
>>  
>>  # With feature table containing bit 63
>> @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear 
>> feature bit ===
>>  echo
>>  _make_test_img 64M
>>  $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63
>> -$PYTHON qcow2.py "$TEST_IMG" dump-header
>> +$PYTHON qcow2.py "$TEST_IMG" dump-header \
>> +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
>>  
>>  echo
>>  echo === Repair image ===
>>  echo
>>  _check_test_img -r all
>>  
>> -$PYTHON qcow2.py "$TEST_IMG" dump-header
>> +$PYTHON qcow2.py "$TEST_IMG" dump-header \
>> +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
>>  
>>  # success, all done
>>  echo "*** done"
>> diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
>> index e489b44386..998c2a8a35 100644
>> --- a/tests/qemu-iotests/036.out
>> +++ b/tests/qemu-iotests/036.out
>> @@ -19,7 +19,7 @@ snapshot_offset   0x0
>>  incompatible_features 0x8000
>>  compatible_features   0x0
>>  autoclear_features0x0
>> -refcount_order4
>> +refcount_order(filtered)
>>  header_length 104
>>  
>>  qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT 
>> feature(s): Unknown incompatible feature: 8000
>> @@ -53,7 +53,7 @@ snapshot_offset   0x0
>>  incompatible_features 0x0
>>  compatible_features   0x0
>>  autoclear_features0x8000
>> -refcount_order4
>> +refcount_order(filtered)
>>  header_length 104
>>  
>>  Header extension:
>> @@ -81,7 +81,7 @@ snapshot_offset   0x0
>>  incompatible_features 0x0
>>  compatible_features   0x0
>>  autoclear_features0x0
>> -refcount_order4
>> +refcount_order(filtered)
>>  header_length 104
>>  
>>  Header extension:
>
> Minor notes:
>
> 1. Maybe put the sed command into a function to avoid duplication?

 Hm, maybe, but that would increase the LoC, so I’m not sure whether it
 really would be a simplification.
>>>
>>> IMHO, in my opinion, regardless of LOC, duplicated code is almost always
>>> bad. Common functions are mostly for the next guy that will be able to use
>>> them instead of searching through code to see how this is done there and 
>>> there.
>>
>> In my experience, people write tests without scanning common.filter for
>> whether anyone has written the same code already.  See the
>> _filter_qemu_img_check example in this series.
> Fair enough, but this can change.

It won’t.

It only changes when people use standardized functions that filter
automatically.  And writing that made me look into common.rc, and ah
yes, we have a _check_test_img.  So the correct thing to do is not to
use _filter_qemu_img_check (or to expect people to scan the filter file
for filters), but to use _check_test_img there.

> 2. As I understand the test, it only checks the feature bits.
>So maybe instead of blacklisting some of the values, white list
>only the feature bits in the output?

 Hm.  Seems reasonable, but then again I’d prefer a minimal change, and
 changing it to a whitelist would be more change...
>>>
>>> I don't think this is bad, again in my eyes, the code should be as friendly
>>> as possible for the next guy that will have to change it, so adding
>>> some more extra changes don't seem a problem to me.
>>
>> In my eyes tests aren’

Re: [PATCH 15/18] iotests: Make 137 work with data_file

[Maxim Levitsky]

On Mon, 2019-09-30 at 15:57 +0200, Max Reitz wrote:
> On 30.09.19 15:46, Maxim Levitsky wrote:
> > On Mon, 2019-09-30 at 15:38 +0200, Max Reitz wrote:
> > > On 29.09.19 18:38, Maxim Levitsky wrote:
> > > > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
> > > > > When using an external data file, there are no refcounts for data
> > > > > clusters.  We thus have to adjust the corruption test in this patch to
> > > > > not be based around a data cluster allocation, but the L2 table
> > > > > allocation (L2 tables are still refcounted with external data files).
> > > > > 
> > > > > Doing so means this test works both with and without external data
> > > > > files.
> > > > > 
> > > > > Signed-off-by: Max Reitz 
> > > > > ---
> > > > >  tests/qemu-iotests/137 | 10 ++
> > > > >  tests/qemu-iotests/137.out |  4 +---
> > > > >  2 files changed, 7 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
> > > > > index 6cf2997577..dd3484205e 100755
> > > > > --- a/tests/qemu-iotests/137
> > > > > +++ b/tests/qemu-iotests/137
> > > > > @@ -138,14 +138,16 @@ $QEMU_IO \
> > > > >  "$TEST_IMG" 2>&1 | _filter_qemu_io
> > > > >  
> > > > >  # The dirty bit must not be set
> > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
> > > > > +# (Filter the external data file bit)
> > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep 
> > > > > incompatible_features \
> > > > > +| sed -e 's/0x4/0x0/'
> > > > 
> > > > Maybe it is better to filter all the feature bits, but the dirty bit,
> > > > since only it is needed here, so that when we start running tests with
> > > > more features, we won't need to do this again?
> > > 
> > > I’d hate a filter s/[02468ace]$/no dirty bit/ though.
> > 
> > Nothing a helper function can't solve IMHO, I would convert this to a 
> > number,
> > and then check bitwise the bit 2, assuming that is the dirty bit)
> > Again, note that my approach to code is to make it as easy as possible for 
> > the
> > next guy to change, so I am noticing such places. Eventually someone of us,
> > will be that next guy. Then again, I don't mind leaving this as is, just 
> > noting this.
> 
> Again, my approach to tests is that they aren’t classical code.
> 
> This is a very personal opinion, but I have found that tests that have
> the most ad-hoc code with the least function calls are the easiest to
> work with.
> 
> Tests that have a whole lot of infrastructure and try to have nice code
> are a horror to work with because you first have to understand how they
> work.

I guess everything should be in balance, but I understand very well
what you mean.

> 
> Tests should be simple, not complex.  Some ad-hoc filters do not make
> them complex as long as it’s obvious what they do.

Yea, but the point is that it makes it harder to test new features,
that slightly change the output of the tests, and break various tests that 
hardcode unneeded things.

Pretty much exactly what this patch series does, 
is to remove some of the ad-hoc stuff to make the
tests work with a new feature. 

A bit more generic tests, might be able
to reduce this work. Anyway I won't argue with this as well, its all matter of
balance, both extremes are no doubt bad.

> 
> 
> Also, the correct approach here is not to do number crunching in bash.
> It is to change qcow2.py to emit more easily filterable information.
100% agree. 

> 
> Max
> 

Best regards,
Maxim Levitsky

Re: [RFC] cpu_map: Remove pconfig from Icelake-Server CPU model

[Eduardo Habkost]

CCing qemu-devel and QEMU developers.

On Mon, Sep 30, 2019 at 12:24:53PM +0200, Jiri Denemark wrote:
> On Thu, Sep 26, 2019 at 18:43:05 -0300, Eduardo Habkost wrote:
> > The pconfig feature never worked, and adding "pconfig=off" to the
> > QEMU command-line triggers a regression in QEMU 3.1.1 and 4.0.0.
> > 
> > Signed-off-by: Eduardo Habkost 
> > ---
> > I'm sending this as an RFC because I couldn't test it properly,
> > and because I don't know what are the consequences of changing
> > cpu_map between libvirt versions.
> > ---
> >  src/cpu_map/x86_Icelake-Server.xml | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/cpu_map/x86_Icelake-Server.xml 
> > b/src/cpu_map/x86_Icelake-Server.xml
> > index fb15977a59..188a781282 100644
> > --- a/src/cpu_map/x86_Icelake-Server.xml
> > +++ b/src/cpu_map/x86_Icelake-Server.xml
> > @@ -56,7 +56,9 @@
> >  
> >  
> >  
> > -
> > +
> >  
> >  
> >  
> 
> IIUC this never worked and a domain started with Icelake-Server CPU
> model would actually end up running with pconfig=off, right? In that
> case removing pconfig from Icelake-Server would not cause any issues
> when a domain is started. However, I'm afraid migration would be broken.
> 
> If a domain is started by new libvirt (with this patch in) using
> Icelake-Server CPU model possibly with additional features added or
> removed, but without pconfig being explicitly mentioned, libvirt would
> not disable pconfig when updating active definition according to the
> actual CPU created by QEMU. This is because libvirt did not ask for
> pconfig (either explicitly or implicitly via the CPU model). When such
> domain gets migrated to an older libvirt (which thinks pconfig is part
> of Icelake-Server), it will complain that QEMU disabled pconfig while
> the source domain was running with pconfig enabled (which is an
> incorrect result caused by inconsistent view of the CPU model).
> 
> Thus we would need to add a hack which would explicitly disable pconfig
> in the domain definition used for migration to make sure the destination
> libvirtd knows pconfig was disabled. New libvirt would just drop the
> disabled pconfig feature from the CPU definition before starting a
> domain.
> 
> [1] From this point of view we could just keep the CPU model in libvirt
> untouched. This way pconfig would always be explicitly disabled when a
> domain is running and the host-model CPU definition would also show it
> as explicitly disabled.
> 
> [2] However starting a domain with Icelake-Server so that it can be
> migrated or saved/restored on QEMU in 3.1.1 and 4.0.0 would be
> impossible. This can be solved by a different hack, which would drop
> pconfig=off from QEMU command line.
> 
> [3] But if pconfig was removed from QEMU and never returned back, we
> could remove it from any domain XML we see (virQEMUCapsCPUFilterFeatures
> mentions several other features which we handle this way).
> 
> That said, I think [3] would be the best option. But if QEMU cannot or
> doesn't want to remove pconfig completely, I'd go with [1] as the hack
> in [2] would be too ugly and confusing.

>From the QEMU side, [3] is better, as pconfig was added by
accident in 3.1.0 and it would be simpler to not re-add it.

This might be a problem if there are plans to eventually make KVM
support pconfig, though.  Paolo, Robert, are there plans to
support pconfig in KVM in the future?

-- 
Eduardo

Re: [PATCH 01/18] iotests: Filter refcount_order in 036

[Maxim Levitsky]

On Mon, 2019-09-30 at 16:04 +0200, Max Reitz wrote:
> On 30.09.19 15:58, Maxim Levitsky wrote:
> > On Mon, 2019-09-30 at 15:44 +0200, Max Reitz wrote:
> > > On 30.09.19 15:40, Maxim Levitsky wrote:
> > > > On Mon, 2019-09-30 at 14:43 +0200, Max Reitz wrote:
> > > > > On 29.09.19 18:31, Maxim Levitsky wrote:
> > > > > > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
> > > > > > > This test can run just fine with other values for refcount_bits, 
> > > > > > > so we
> > > > > > > should filter the value from qcow2.py's dump-header.
> > > > > > > 
> > > > > > > (036 currently ignores user-specified image options, but that 
> > > > > > > will be
> > > > > > > fixed in the next patch.)
> > > > > > > 
> > > > > > > Signed-off-by: Max Reitz 
> > > > > > > ---
> > > > > > >  tests/qemu-iotests/036 | 9 ++---
> > > > > > >  tests/qemu-iotests/036.out | 6 +++---
> > > > > > >  2 files changed, 9 insertions(+), 6 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
> > > > > > > index f06ff67408..69d0f9f903 100755
> > > > > > > --- a/tests/qemu-iotests/036
> > > > > > > +++ b/tests/qemu-iotests/036
> > > > > > > @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit 
> > > > > > > incompatible 63
> > > > > > >  
> > > > > > >  # Without feature table
> > > > > > >  $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857
> > > > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header
> > > > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \
> > > > > > > +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
> > > > > > >  _img_info
> > > > > > >  
> > > > > > >  # With feature table containing bit 63
> > > > > > > @@ -103,14 +104,16 @@ echo === Create image with unknown 
> > > > > > > autoclear feature bit ===
> > > > > > >  echo
> > > > > > >  _make_test_img 64M
> > > > > > >  $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63
> > > > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header
> > > > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \
> > > > > > > +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
> > > > > > >  
> > > > > > >  echo
> > > > > > >  echo === Repair image ===
> > > > > > >  echo
> > > > > > >  _check_test_img -r all
> > > > > > >  
> > > > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header
> > > > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \
> > > > > > > +| sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
> > > > > > >  
> > > > > > >  # success, all done
> > > > > > >  echo "*** done"
> > > > > > > diff --git a/tests/qemu-iotests/036.out 
> > > > > > > b/tests/qemu-iotests/036.out
> > > > > > > index e489b44386..998c2a8a35 100644
> > > > > > > --- a/tests/qemu-iotests/036.out
> > > > > > > +++ b/tests/qemu-iotests/036.out
> > > > > > > @@ -19,7 +19,7 @@ snapshot_offset   0x0
> > > > > > >  incompatible_features 0x8000
> > > > > > >  compatible_features   0x0
> > > > > > >  autoclear_features0x0
> > > > > > > -refcount_order4
> > > > > > > +refcount_order(filtered)
> > > > > > >  header_length 104
> > > > > > >  
> > > > > > >  qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT 
> > > > > > > feature(s): Unknown incompatible feature: 8000
> > > > > > > @@ -53,7 +53,7 @@ snapshot_offset   0x0
> > > > > > >  incompatible_features 0x0
> > > > > > >  compatible_features   0x0
> > > > > > >  autoclear_features0x8000
> > > > > > > -refcount_order4
> > > > > > > +refcount_order(filtered)
> > > > > > >  header_length 104
> > > > > > >  
> > > > > > >  Header extension:
> > > > > > > @@ -81,7 +81,7 @@ snapshot_offset   0x0
> > > > > > >  incompatible_features 0x0
> > > > > > >  compatible_features   0x0
> > > > > > >  autoclear_features0x0
> > > > > > > -refcount_order4
> > > > > > > +refcount_order(filtered)
> > > > > > >  header_length 104
> > > > > > >  
> > > > > > >  Header extension:
> > > > > > 
> > > > > > Minor notes:
> > > > > > 
> > > > > > 1. Maybe put the sed command into a function to avoid duplication?
> > > > > 
> > > > > Hm, maybe, but that would increase the LoC, so I’m not sure whether it
> > > > > really would be a simplification.
> > > > 
> > > > IMHO, in my opinion, regardless of LOC, duplicated code is almost always
> > > > bad. Common functions are mostly for the next guy that will be able to 
> > > > use
> > > > them instead of searching through code to see how this is done there 
> > > > and there.
> > > 
> > > In my experience, people write tests without scanning common.filter for
> > > whether anyone has written the same code already.  See the
> > > _filter_qemu_img_check example in this series.
> > 
> > Fair enough, but this can change.
> 
> It won’t.
> 
> It only changes when people use standardized functions that filter
> automatically.  And writing that made me look i

RE: [edk2-devel] [Qemu-devel] [PATCH 1/2] q35: implement 128K SMRAM at default SMBASE address

[Yao, Jiewen]

below

> -Original Message-
> From: de...@edk2.groups.io  On Behalf Of Igor
> Mammedov
> Sent: Monday, September 30, 2019 8:37 PM
> To: Laszlo Ersek 
> Cc: de...@edk2.groups.io; qemu-devel@nongnu.org; Chen, Yingwen
> ; phillip.go...@oracle.com;
> alex.william...@redhat.com; Yao, Jiewen ; Nakajima,
> Jun ; Kinney, Michael D
> ; pbonz...@redhat.com;
> boris.ostrov...@oracle.com; r...@edk2.groups.io; joao.m.mart...@oracle.com;
> Brijesh Singh 
> Subject: Re: [edk2-devel] [Qemu-devel] [PATCH 1/2] q35: implement 128K
> SMRAM at default SMBASE address
> 
> On Mon, 30 Sep 2019 13:51:46 +0200
> "Laszlo Ersek"  wrote:
> 
> > Hi Igor,
> >
> > On 09/24/19 13:19, Igor Mammedov wrote:
> > > On Mon, 23 Sep 2019 20:35:02 +0200
> > > "Laszlo Ersek"  wrote:
> >
> > >> I've got good results. For this (1/2) QEMU patch:
> > >>
> > >> Tested-by: Laszlo Ersek 
> > >>
> > >> I tested the following scenarios. In every case, I verified the OVMF
> > >> log, and also the "info mtree" monitor command's result (i.e. whether
> > >> "smbase-blackhole" / "smbase-window" were disabled or enabled).
> > >> Mostly, I diffed these text files between the test scenarios (looking
> > >> for desired / undesired differences). In the Linux guests, I checked
> > >> / compared the dmesg too (wrt. the UEFI memmap).
> > >>
> > >> - unpatched OVMF (regression test), Fedora guest, normal boot and S3
> > >>
> > >> - patched OVMF, but feature disabled with "-global
> > >>   mch.smbase-smram=off" (another regression test), Fedora guest,
> > >>   normal boot and S3
> > >>
> > >> - patched OVMF, feature enabled, Fedora and various Windows guests
> > >>   (win7, win8, win10 families, client/server), normal boot and S3
> > >>
> > >> - a subset of the above guests, with S3 disabled (-global
> > >>   ICH9-LPC.disable_s3=1), and obviously S3 resume not tested
> > >>
> > >> SEV: used 5.2-ish Linux guest, with S3 disabled (no support under SEV
> > >> for that now):
> > >>
> > >> - unpatched OVMF (regression test), normal boot
> > >>
> > >> - patched OVMF but feature disabled on the QEMU cmdline (another
> > >>   regression test), normal boot
> > >>
> > >> - patched OVMF, feature enabled, normal boot.
> > >>
> > >> I plan to post the OVMF patches tomorrow, for discussion.
> > >>
> > >> (It's likely too early to push these QEMU / edk2 patches right now --
> > >> we don't know yet if this path will take us to the destination. For
> > >> now, it certainly looks great.)
> > >
> > > Laszlo, thanks for trying it out.
> > > It's nice to hear that approach is somewhat usable.
> > > Hopefully we won't have to invent 'paused' cpu mode.
> > >
> > > Pls CC me on your patches
> > > (not that I qualify for reviewing,
> > > but may be I could learn a thing or two from it)
> >
> > Considering the plan at [1], the two patch sets [2] [3] should cover
> > step (01); at least as proof of concept.
> >
> > [1] [edk2-devel] CPU hotplug using SMM with QEMU+OVMF
> > http://mid.mail-archive.com/20190830164802.1b17ff26@redhat.com
> >
> > [2] The current thread:
> > [Qemu-devel] [PATCH 0/2] q35: mch: allow to lock down 128K RAM at
> default SMBASE address
> > http://mid.mail-archive.com/20190917130708.10281-1-
> imamm...@redhat.com
> >
> > [3] [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default
> SMBASE" feature
> > http://mid.mail-archive.com/20190924113505.27272-1-lersek@redhat.com
> >
> > (I'll have to figure out what SMI handler to put in place there, but I'd
> > like to experiment with that once we can cause a new CPU to start
> > executing code there, in SMM.)
> >
> > So what's next?
> >
> > To me it looks like we need to figure out how QEMU can make the OS call
> > into SMM (in the GPE cpu hotplug handler), passing in parameters and
> > such. This would be step (03).
> >
> > Do you agree?
> >
> > If so, I'll ask Jiewen about such OS->SMM calls separately, because I
> > seem to remember that there used to be an "SMM communcation table" of
> > sorts, for flexible OS->SMM calls. However, it appears to be deprecated
> > lately.
> we can try to resurrect and put over it some kind of protocol
> to describe which CPUs to where hotplugged.
> 
> or we could put a parameter into SMI status register (IO port 0xb3)
> and the trigger SMI from GPE handler to tell SMI handler that cpu
> hotplug happened and then use QEMU's cpu hotplug interface
> to enumerate hotplugged CPUs for SMI handler.
> 
> The later is probably simpler as we won't need to reinvent the wheel
> (just reuse the interface that's already in use by GPE handler).

[Jiewen] The PI specification Volume 4 - SMM defines 
EFI_MM_COMMUNICATION_PROTOCOL.Communicate() - It can be used to communicate 
between OS and SMM handler. But it requires the runtime protocol call. I am not 
sure how OS loader passes this information to OS kernel.

As such, I think using ACPI SCI/GPE -> software SMI handler is an easier way to 
achieve this. I also recommend this way.
For parameter passing, we can use 1) Port B2 (1 b

Re: Thoughts on VM fence infrastructure

[Dr. David Alan Gilbert]

* Felipe Franciosi (fel...@nutanix.com) wrote:
> Heyall,
> 
> We have a use case where a host should self-fence (and all VMs should
> die) if it doesn't hear back from a heartbeat within a certain time
> period. Lots of ideas were floated around where libvirt could take
> care of killing VMs or a separate service could do it. The concern
> with those is that various failures could lead to _those_ services
> being unavailable and the fencing wouldn't be enforced as it should.
> 
> Ultimately, it feels like Qemu should be responsible for this
> heartbeat and exit (or execute a custom callback) on timeout.

It doesn't feel doing it inside qemu would be any safer;  something
outside QEMU can forcibly emit a kill -9 and qemu *will* stop.

> Does something already exist for this purpose which could be used?
> Would a generic Qemu-fencing infrastructure be something of interest?
Dave


> Cheers,
> F.
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 07/18] iotests: Replace IMGOPTS= by -o

[Maxim Levitsky]

On Mon, 2019-09-30 at 15:00 +0200, Max Reitz wrote:
> On 29.09.19 18:33, Maxim Levitsky wrote:
> > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
> > > Tests should not overwrite all user-supplied image options, but only add
> > > to it (which will effectively overwrite conflicting values).  Accomplish
> > > this by passing options to _make_test_img via -o instead of $IMGOPTS.
> > > 
> > > For some tests, there is no functional change because they already only
> > > appended options to IMGOPTS.  For these, this patch is just a
> > > simplification.
> > > 
> > > For others, this is a change, so they now heed user-specified $IMGOPTS.
> > > Some of those tests do not work with all image options, though, so we
> > > need to disable them accordingly.
> > > 
> > > Signed-off-by: Max Reitz 
> > > ---
> > >  tests/qemu-iotests/031 |  9 ---
> > >  tests/qemu-iotests/039 | 24 ++
> > >  tests/qemu-iotests/059 | 18 ++---
> > >  tests/qemu-iotests/060 |  6 ++---
> > >  tests/qemu-iotests/061 | 57 ++
> > >  tests/qemu-iotests/079 |  3 +--
> > >  tests/qemu-iotests/106 |  2 +-
> > >  tests/qemu-iotests/108 |  2 +-
> > >  tests/qemu-iotests/112 | 32 
> > >  tests/qemu-iotests/115 |  3 +--
> > >  tests/qemu-iotests/121 |  6 ++---
> > >  tests/qemu-iotests/125 |  2 +-
> > >  tests/qemu-iotests/137 |  2 +-
> > >  tests/qemu-iotests/138 |  3 +--
> > >  tests/qemu-iotests/175 |  2 +-
> > >  tests/qemu-iotests/190 |  2 +-
> > >  tests/qemu-iotests/191 |  3 +--
> > >  tests/qemu-iotests/220 |  4 ++-
> > >  tests/qemu-iotests/243 |  6 +++--
> > >  tests/qemu-iotests/244 | 10 +---
> > >  tests/qemu-iotests/250 |  3 +--
> > >  tests/qemu-iotests/265 |  2 +-
> > >  22 files changed, 100 insertions(+), 101 deletions(-)
> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> > > index 325da63a4c..99563bf126 100755
> > > --- a/tests/qemu-iotests/039
> > > +++ b/tests/qemu-iotests/039
> > > @@ -50,8 +50,7 @@ size=128M
> > >  echo
> > >  echo "== Checking that image is clean on shutdown =="
> > >  
> > > -IMGOPTS="compat=1.1,lazy_refcounts=on"
> > > -_make_test_img $size
> > > +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size
> > 
> > Any reason for compat=1.1 here? Other that it was before like that.
> 
> Lazy refcounts only work with qcow2 v3 (compat=1.1).
Agree, sorry for not noticing this.

Best regards,
Maxim Levitsky

> 
> Max
> 

Re: [PATCH 09/18] iotests: Drop IMGOPTS use in 267

[Maxim Levitsky]

On Mon, 2019-09-30 at 15:01 +0200, Max Reitz wrote:
> On 29.09.19 18:33, Maxim Levitsky wrote:
> > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
> > > Overwriting IMGOPTS means ignoring all user-supplied options, which is
> > > not what we want.  Replace the current IMGOPTS use by a new BACKING_FILE
> > > variable.
> > > 
> > > Signed-off-by: Max Reitz 
> > > ---
> > >  tests/qemu-iotests/267 | 12 
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
> > > index 95f885442f..529f5f9afe 100755
> > > --- a/tests/qemu-iotests/267
> > > +++ b/tests/qemu-iotests/267
> > > @@ -68,7 +68,11 @@ size=128M
> > >  
> > >  run_test()
> > >  {
> > > -_make_test_img $size
> > > +if [ -n "$BACKING_FILE" ]; then
> > > +_make_test_img -b "$BACKING_FILE" $size
> > > +else
> > > +_make_test_img $size
> > > +fi
> > >  printf "savevm snap0\ninfo snapshots\nloadvm snap0\n" | run_qemu 
> > > "$@" | _filter_date
> > >  }
> > >  
> > > @@ -119,12 +123,12 @@ echo
> > >  
> > >  TEST_IMG="$TEST_IMG.base" _make_test_img $size
> > >  
> > > -IMGOPTS="backing_file=$TEST_IMG.base" \
> > > +BACKING_FILE="$TEST_IMG.base" \
> > >  run_test -blockdev 
> > > driver=file,filename="$TEST_IMG.base",node-name=backing-file \
> > >   -blockdev driver=file,filename="$TEST_IMG",node-name=file \
> > >   -blockdev 
> > > driver=$IMGFMT,file=file,backing=backing-file,node-name=fmt
> > >  
> > > -IMGOPTS="backing_file=$TEST_IMG.base" \
> > > +BACKING_FILE="$TEST_IMG.base" \
> > >  run_test -blockdev 
> > > driver=file,filename="$TEST_IMG.base",node-name=backing-file \
> > >   -blockdev 
> > > driver=$IMGFMT,file=backing-file,node-name=backing-fmt \
> > >   -blockdev driver=file,filename="$TEST_IMG",node-name=file \
> > > @@ -141,7 +145,7 @@ echo
> > >  echo "=== -blockdev with NBD server on the backing file ==="
> > >  echo
> > >  
> > > -IMGOPTS="backing_file=$TEST_IMG.base" _make_test_img $size
> > > +_make_test_img -b "$TEST_IMG.base" $size
> > >  cat < > >  nbd_server_start unix:$TEST_DIR/nbd
> > >  nbd_server_add -w backing-fmt
> > 
> > qemu's master (pulled today), don't have iotest 267,
> > you probably based this on some not yet merged branch.
> > Or I made some mistake with pulling the master branch.
> 
> Yep, it’s only in Kevin’s block branch (and thus mine, too).  He sent a
> pull request for it, which was rejected though (because this test is
> broken on anything but x64, but that doesn’t stop this patch).
> 

I figured out that it must be something like that.
Being lazy, I would like to ask if we have maybe a list of these 
branches somewhere in the wiki?

Best regards,
Maxim Levitsky

RE: [RFC] cpu_map: Remove pconfig from Icelake-Server CPU model

[Hu, Robert]

> -Original Message-
> From: Eduardo Habkost 
> Sent: Monday, September 30, 2019 22:11
> To: Jiri Denemark 
> Cc: libvir-l...@redhat.com; qemu-devel@nongnu.org; Paolo Bonzini
> ; Daniel P. Berrangé ; Kang,
> Luwei ; thomas.lenda...@amd.com; Robert Hoo
> ; Richard Henderson ; Jiri
> Denemark ; Huang, Kai ; Hu,
> Robert 
> Subject: Re: [RFC] cpu_map: Remove pconfig from Icelake-Server CPU model
> 
> CCing qemu-devel and QEMU developers.
> 
> On Mon, Sep 30, 2019 at 12:24:53PM +0200, Jiri Denemark wrote:
> > On Thu, Sep 26, 2019 at 18:43:05 -0300, Eduardo Habkost wrote:
> > > The pconfig feature never worked, and adding "pconfig=off" to the
> > > QEMU command-line triggers a regression in QEMU 3.1.1 and 4.0.0.
> > >
> > > Signed-off-by: Eduardo Habkost 
> > > ---
> > > I'm sending this as an RFC because I couldn't test it properly, and
> > > because I don't know what are the consequences of changing cpu_map
> > > between libvirt versions.
> > > ---
> > >  src/cpu_map/x86_Icelake-Server.xml | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/cpu_map/x86_Icelake-Server.xml
> > > b/src/cpu_map/x86_Icelake-Server.xml
> > > index fb15977a59..188a781282 100644
> > > --- a/src/cpu_map/x86_Icelake-Server.xml
> > > +++ b/src/cpu_map/x86_Icelake-Server.xml
> > > @@ -56,7 +56,9 @@
> > >  
> > >  
> > >  
> > > -
> > > +
> > >  
> > >  
> > >  
> >
> > IIUC this never worked and a domain started with Icelake-Server CPU
> > model would actually end up running with pconfig=off, right? In that
> > case removing pconfig from Icelake-Server would not cause any issues
> > when a domain is started. However, I'm afraid migration would be broken.
> >
> > If a domain is started by new libvirt (with this patch in) using
> > Icelake-Server CPU model possibly with additional features added or
> > removed, but without pconfig being explicitly mentioned, libvirt would
> > not disable pconfig when updating active definition according to the
> > actual CPU created by QEMU. This is because libvirt did not ask for
> > pconfig (either explicitly or implicitly via the CPU model). When such
> > domain gets migrated to an older libvirt (which thinks pconfig is part
> > of Icelake-Server), it will complain that QEMU disabled pconfig while
> > the source domain was running with pconfig enabled (which is an
> > incorrect result caused by inconsistent view of the CPU model).
> >
> > Thus we would need to add a hack which would explicitly disable
> > pconfig in the domain definition used for migration to make sure the
> > destination libvirtd knows pconfig was disabled. New libvirt would
> > just drop the disabled pconfig feature from the CPU definition before
> > starting a domain.
> >
> > [1] From this point of view we could just keep the CPU model in
> > libvirt untouched. This way pconfig would always be explicitly
> > disabled when a domain is running and the host-model CPU definition
> > would also show it as explicitly disabled.
> >
> > [2] However starting a domain with Icelake-Server so that it can be
> > migrated or saved/restored on QEMU in 3.1.1 and 4.0.0 would be
> > impossible. This can be solved by a different hack, which would drop
> > pconfig=off from QEMU command line.
> >
> > [3] But if pconfig was removed from QEMU and never returned back, we
> > could remove it from any domain XML we see
> > (virQEMUCapsCPUFilterFeatures mentions several other features which we
> handle this way).
> >
> > That said, I think [3] would be the best option. But if QEMU cannot or
> > doesn't want to remove pconfig completely, I'd go with [1] as the hack
> > in [2] would be too ugly and confusing.
> 
> From the QEMU side, [3] is better, as pconfig was added by accident in 3.1.0 
> and
> it would be simpler to not re-add it.
> 
> This might be a problem if there are plans to eventually make KVM support
> pconfig, though.  Paolo, Robert, are there plans to support pconfig in KVM in 
> the
> future?
[Robert Hoo] 
Thanks Eduardo for efforts in resolving this issue, introduced from my Icelake 
CPU
model patch.
I've no idea about PCONFIG's detail and plan. Let me sync with Huang, Kai and 
answer
you soon.
> 
> --
> Eduardo

Re: Re: target/ppc: bug in optimised vsl/vsr implementation?

[Paul Clarke]

On 9/28/19 5:17 PM, Aleksandar Markovic wrote:
> Also, check on the hardware the behavior listed as 'undefined' for vsl/vsr
> in the docs - even though it is tehnically irrelevant, I am courious
> whether the old or the new (or none of them) solution match the hardware.

There does appear to be some odd behavior when one strays into the undefined.  
For example:
source vector: 0102030405060708090a0b0c0d0e0f10
shift  vector: 01020101010101010101010101010101
after vsl: 020806080a0c0e10121416181a1c1e20
...this appears to use the byte-respective shift values

using vsr with that result and the same shift vector:
after vsr: 0182030405060708090a0b0c0d0e0f10
I expected to get back a result matching the source vector, but somehow, an 
extra bit got set.

It would probably take some more thorough investigation to map out the 
undefined behavior, but I doubt there's any value to that.

PC

Re: target/ppc: bug in optimised vsl/vsr implementation?

[Aleksandar Markovic]

> 5. (a question for Mark) After all recent changes, does get_avr64(...,
..., true) always (for any endian configuration) return the "high" half of
an Altivec register, and get_avr64(..., ..., false) the "low" one?
>
> Given all these circumstances, perhaps the most reasonable solution would
be to revert the commit in question, and allow Stefan enough dev and test
time to hopefully submit a new, better, version later on.
>

Mark, can you verify please that this is true? The thing is, 'vsl/vsr'
belong to the group of SIMD instructions where the distinction between
'high' and 'low' 64-bit halves of the source and destination registers is
important (as opposed to the majority of 'regular' SIMD instructions, like
vector addition, vector max/min, etc., that act only as parallel operations
on corresdponding data elements).

Regards, Aleksandar

> Sincerely,
> Aleksandar
>

Re: [PATCH 0/5] TriCore fixes and gdbstub

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20190930124643.179695-1-kbast...@mail.uni-paderborn.de/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20190930124643.179695-1-kbast...@mail.uni-paderborn.de
Subject: [PATCH 0/5] TriCore fixes and gdbstub

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
e0ab359 target/tricore: Implement gdbstub
af906d6 target/tricore: Implement tricore_cpu_get_phys_page_debug
421ac82 target/tricore: Raise EXCP_DEBUG in gen_goto_tb() for singlestep
5a6c845 target/tricore: Move translate feature check to ctx
69f10b4 target/tricore: Don't save pc in generate_qemu_excp

=== OUTPUT BEGIN ===
1/5 Checking commit 69f10b4a4d39 (target/tricore: Don't save pc in 
generate_qemu_excp)
2/5 Checking commit 5a6c84545f56 (target/tricore: Move translate feature check 
to ctx)
3/5 Checking commit 421ac8270548 (target/tricore: Raise EXCP_DEBUG in 
gen_goto_tb() for singlestep)
4/5 Checking commit af906d6437cd (target/tricore: Implement 
tricore_cpu_get_phys_page_debug)
5/5 Checking commit e0ab359a6c96 (target/tricore: Implement gdbstub)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#59: 
new file mode 100644

WARNING: line over 80 characters
#133: FILE: target/tricore/gdbstub.c:70:
+static void tricore_cpu_gdb_write_csfr(CPUTriCoreState *env, int n, uint32_t 
val)

ERROR: spaces required around that '-' (ctx:VxV)
#178: FILE: target/tricore/gdbstub.c:115:
+return gdb_get_reg32(mem_buf, env->gpr_a[n-16]);
   ^

ERROR: spaces required around that '-' (ctx:VxV)
#196: FILE: target/tricore/gdbstub.c:133:
+env->gpr_d[n-16] = tmp;
 ^

total: 2 errors, 2 warnings, 170 lines checked

Patch 5/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190930124643.179695-1-kbast...@mail.uni-paderborn.de/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH 02/18] iotests: Replace IMGOPTS by _unsupported_imgopts

[Maxim Levitsky]

On Mon, 2019-09-30 at 14:59 +0200, Max Reitz wrote:
> On 29.09.19 18:31, Maxim Levitsky wrote:
> > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
> > > Some tests require compat=1.1 and thus set IMGOPTS='compat=1.1'
> > > globally.  That is not how it should be done; instead, they should
> > > simply set _unsupported_imgopts to compat=0.10 (compat=1.1 is the
> > > default anyway).
> > > 
> > > This makes the tests heed user-specified $IMGOPTS.  Some do not work
> > > with all image options, though, so we need to disable them accordingly.
> > > 
> > > Signed-off-by: Max Reitz 
> > > ---
> > >  tests/qemu-iotests/036 | 3 +--
> > >  tests/qemu-iotests/060 | 4 ++--
> > >  tests/qemu-iotests/062 | 3 ++-
> > >  tests/qemu-iotests/066 | 3 ++-
> > >  tests/qemu-iotests/068 | 3 ++-
> > >  tests/qemu-iotests/098 | 3 +--
> > >  6 files changed, 10 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
> > > index 69d0f9f903..57dda23b02 100755
> > > --- a/tests/qemu-iotests/036
> > > +++ b/tests/qemu-iotests/036
> > > @@ -43,9 +43,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  # This tests qcow2-specific low-level functionality
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > > -
> > >  # Only qcow2v3 and later supports feature bits
> > > -IMGOPTS="compat=1.1"
> > > +_unsupported_imgopts 'compat=0.10'
> > >  
> > >  echo
> > >  echo === Image with unknown incompatible feature bit ===
> > > diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
> > > index b91d8321bb..9c2ef42522 100755
> > > --- a/tests/qemu-iotests/060
> > > +++ b/tests/qemu-iotests/060
> > > @@ -48,6 +48,8 @@ _filter_io_error()
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > >  _supported_os Linux
> > > +# These tests only work for compat=1.1 images with refcount_bits=16
> > > +_unsupported_imgopts 'compat=0.10' 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> > >  
> > >  rt_offset=65536  # 0x1 (XXX: just an assumption)
> > >  rb_offset=131072 # 0x2 (XXX: just an assumption)
> > > @@ -55,8 +57,6 @@ l1_offset=196608 # 0x3 (XXX: just an assumption)
> > >  l2_offset=262144 # 0x4 (XXX: just an assumption)
> > >  l2_offset_after_snapshot=524288 # 0x8 (XXX: just an assumption)
> > >  
> > > -IMGOPTS="compat=1.1"
> > > -
> > >  OPEN_RW="open -o overlap-check=all $TEST_IMG"
> > >  # Overlap checks are done before write operations only, therefore 
> > > opening an
> > >  # image read-only makes the overlap-check option irrelevant
> > > diff --git a/tests/qemu-iotests/062 b/tests/qemu-iotests/062
> > > index d5f818fcce..ac0d2a9a3b 100755
> > > --- a/tests/qemu-iotests/062
> > > +++ b/tests/qemu-iotests/062
> > > @@ -40,8 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  # This tests qocw2-specific low-level functionality
> > >  _supported_fmt qcow2
> > >  _supported_proto generic
> > > +# We need zero clusters and snapshots
> > > +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
> > >  
> > > -IMGOPTS="compat=1.1"
> > >  IMG_SIZE=64M
> > >  
> > >  echo
> > > diff --git a/tests/qemu-iotests/066 b/tests/qemu-iotests/066
> > > index 28f8c98412..9a15ba8027 100755
> > > --- a/tests/qemu-iotests/066
> > > +++ b/tests/qemu-iotests/066
> > > @@ -39,9 +39,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  # This tests qocw2-specific low-level functionality
> > >  _supported_fmt qcow2
> > >  _supported_proto generic
> > > +# Weneed zero clusters and snapshots
> > 
> > Typo
> 
> Indeed!
> 
> > > +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
> > >  
> > >  # Intentionally create an unaligned image
> > > -IMGOPTS="compat=1.1"
> > >  IMG_SIZE=$((64 * 1024 * 1024 + 512))
> > >  
> > >  echo
> > > diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
> > > index 22f5ca3ba6..65650fca9a 100755
> > > --- a/tests/qemu-iotests/068
> > > +++ b/tests/qemu-iotests/068
> > > @@ -39,8 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  # This tests qocw2-specific low-level functionality
> > >  _supported_fmt qcow2
> > >  _supported_proto generic
> > > +# Internal snapshots are (currently) impossible with refcount_bits=1
> > 
> > Why currently? 1 bit will only allow to mark a cluser as used/unused which
> > is not enough for any snapshots?
> 
> It is, because in theory you can just copy the clusters at the time of
> snapshotting.  We’ll never implement it, but, well...
Fair enough, I didn't even thought of that. I fully agree that
implementing this is silly.

> 
> > > +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
> > >  
> > > -IMGOPTS="compat=1.1"
> > >  IMG_SIZE=128K
> > >  
> > >  case "$QEMU_DEFAULT_MACHINE" in
> > > diff --git a/tests/qemu-iotests/098 b/tests/qemu-iotests/098
> > > index 1c1d1c468f..2d68dc7d6c 100755
> > > --- a/tests/qemu-iotests/098
> > > +++ b/tests/qemu-iotests/098
> > > @@ -40,8 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  
> > >  _supported_fmt qcow2
> > >  _supported_proto file
>

Re: Re: target/ppc: bug in optimised vsl/vsr implementation?

[Aleksandar Markovic]

30.09.2019. 16.35, "Paul Clarke"  је написао/ла:
>
> On 9/28/19 5:17 PM, Aleksandar Markovic wrote:
> > Also, check on the hardware the behavior listed as 'undefined' for
vsl/vsr
> > in the docs - even though it is tehnically irrelevant, I am courious
> > whether the old or the new (or none of them) solution match the
hardware.
>
> There does appear to be some odd behavior when one strays into the
undefined.  For example:
> source vector: 0102030405060708090a0b0c0d0e0f10
> shift  vector: 01020101010101010101010101010101
> after vsl: 020806080a0c0e10121416181a1c1e20
> ...this appears to use the byte-respective shift values
>
> using vsr with that result and the same shift vector:
> after vsr: 0182030405060708090a0b0c0d0e0f10
> I expected to get back a result matching the source vector, but somehow,
an extra bit got set.
>
> It would probably take some more thorough investigation to map out the
undefined behavior, but I doubt there's any value to that.
>

Absolutely agree. I thought if the 'undefined' behavior is something
obviously simple, we could try to match it, assuming also that it remains
constant across all implementations. But, this behaviour is not a simple
one, so, imho, let's leave 'undefined' undefined.

Thanks for a nice experiment!

Aleksandar

> PC