[Qemu-devel] [PATCH] pxa2xx: Add a reset handler to reset the ARM CPU.

[Vijay Kumar B]

Currently the CPU does not get reset when the system_reset command is
invoked. We register a handler in the pxa2xx SoC code, to reset the
CPU as well.

Signed-off-by: Vijay Kumar B. 
Reviewed-by: Deepak S. 
---
 hw/arm/pxa2xx.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 0241e07..08223b7 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -2056,6 +2056,12 @@ static void pxa2xx_reset(void *opaque, int line, int 
level)
 }
 }
 
+static void pxa_reset_cb(void *opaque)
+{
+PXA2xxState *s = opaque;
+cpu_reset(CPU(s->cpu));
+}
+
 /* Initialise a PXA270 integrated chip (ARM based core).  */
 PXA2xxState *pxa270_init(MemoryRegion *address_space,
  unsigned int sdram_size, const char *revision)
@@ -2192,6 +2198,8 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
 /* GPIO1 resets the processor */
 /* The handler can be overridden by board-specific code */
 qdev_connect_gpio_out(s->gpio, 1, s->reset);
+
+qemu_register_reset(pxa_reset_cb, s);
 return s;
 }
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH COLO-Frame (Base) v20 00/17] COarse-grain LOck-stepping(COLO) Virtual Machines for Non-stop Service (FT)

[Amit Shah]

On (Thu) 29 Sep 2016 [16:46:20], zhanghailiang wrote:
> This is the 20th version of COLO frame series.
> 
> COLO Block replication and proxy (COLO compare) series are both been
> merged into upstream already. And further works can be done now to
> realize the full COLO feature.
> 
> Athough there are still no feedbacks from community for this part,
> But I would like to rebase this series to the recent upstream.

Thanks - I see most of the patches are reviewed by at least Dave, with
a couple reviewed by Eric as well.

I know Juan is compiling a pull req, and he'll reach out if there are
concerns.

In the meanwhile, can you check why the autobuilder fails to compile
with your patchset?

Thanks,

Amit

Re: [Qemu-devel] Migration dirty bitmap: should only mark pages as dirty after they have been sent

[Amit Shah]

On (Mon) 26 Sep 2016 [22:55:01], Chunguang Li wrote:
> 
> 
> 
> > -原始邮件-
> > 发件人: "Dr. David Alan Gilbert" 
> > 发送时间: 2016年9月26日 星期一
> > 收件人: "Chunguang Li" 
> > 抄送: qemu-devel@nongnu.org, amit.s...@redhat.com, pbonz...@redhat.com, 
> > stefa...@redhat.com, quint...@redhat.com
> > 主题: Re: [Qemu-devel] Migration dirty bitmap: should only mark pages as 
> > dirty after they have been sent
> > 
> > * Chunguang Li (lichungu...@hust.edu.cn) wrote:
> > > Hi all!
> > > I have some confusion about the dirty bitmap during migration. I have 
> > > digged into the code. I figure out that every now and then during 
> > > migration, the dirty bitmap will be grabbed from the kernel space through 
> > > ioctl(KVM_GET_DIRTY_LOG), and then be used to update qemu's dirty bitmap. 
> > > However I think this mechanism leads to resendness of some NON-dirty 
> > > pages.
> > > 
> > > Take the first iteration of precopy for instance, during which all the 
> > > pages will be sent. Before that during the migration setup, the 
> > > ioctl(KVM_GET_DIRTY_LOG) is called once, so the kernel begins to produce 
> > > the dirty bitmap from this moment. When the pages "that haven't been 
> > > sent" are written, the kernel space marks them as dirty. However I don't 
> > > think this is correct, because these pages will be sent during this and 
> > > the next iterations with the same content (if they are not written again 
> > > after they are sent). It only makes sense to mark the pages which have 
> > > already been sent during one iteration as dirty when they are written.
> > > 
> > > 
> > > Am I right about this consideration? If I am right, is there some advice 
> > > to improve this?
> > 
> > I think you're right that this can happen; to clarify I think the
> > case you're talking about is:
> > 
> >   Iteration 1
> > sync bitmap
> > start sending pages
> > page 'n' is modified - but hasn't been sent yet
> > page 'n' gets sent
> >   Iteration 2
> > sync bitmap
> >'page n is shown as modified'
> > send page 'n' again
> >
> 
> Yes，this is right the case I am talking about.
>  
> > So you're right that is wasteful; I guess it's more wasteful
> > on big VMs with slow networks where the length of each iteration
> > is large.
> 
> I think this is "very" wasteful. Assume the workload writes the pages dirty 
> randomly within the guest address space, and the transfer speed is constant. 
> Intuitively, I think nearly half of the dirty pages produced in Iteration 1 
> is not really dirty. This means the time of Iteration 2 is double of that to 
> send only really dirty pages.

It makes sense, can you get some perf numbers to show what kinds of
workloads get impacted the most?  That would also help us to figure
out what kinds of speed improvements we can expect.


Amit

Re: [Qemu-devel] [PATCH v3 3/8] pci: Convert msix_init() to Error and fix callers to check it

[Cao jin]

On 09/29/2016 10:17 PM, Markus Armbruster wrote:

Cao jin  writes:





-/* Initialize the MSI-X structures */
+/* Make PCI device @dev MSI-X capable
+ * @nentries is the max number of MSI-X vectors that the device support.
+ * @table_bar is the MemoryRegion that MSI-X table structure resides.
+ * @table_bar_nr is number of base address register corresponding to 
@table_bar.
+ * @table_offset indicates the offset that the MSI-X table structure starts 
with
+ * in @table_bar.
+ * @pba_bar is the MemoryRegion that the Pending Bit Array structure resides.
+ * @pba_bar_nr is number of base address register corresponding to @pba_bar.
+ * @pba_offset indicates the offset that the Pending Bit Array structure
+ * starts with in @pba_bar.
+ * Non-zero @cap_pos puts capability MSI-X at that offset in PCI config space.
+ *
+ * Return 0 on success; return -errno on error:


Previous version had:

   + * @errp is for returning errors.
   + *
   + * Return 0 on success; set @errp and return -errno on error.

Intentional change?  I like the old one better.



Oh...it was lost by me. I was planning move these comments into a 
separate patch, but later feel that it is not worth the trouble, so I 
undo the movement, it is lost during the process.




Might conflict with Eric Auger's "Convert VFIO-PCI to realize" series,
but resolving that shouldn't be hard.

[...]

The conversion looks good to me now.


.



--
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v2 7/7] intel-iommu: keep buggy EIM enabled in 2.7 machine type

[Peter Xu]

On Thu, Sep 29, 2016 at 01:23:29PM +0200, Radim Krčmář wrote:

[...]

> @@ -2481,11 +2482,14 @@ static void vtd_realize(DeviceState *dev, Error 
> **errp)
>  if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) {
>  s->intr_eim = ON_OFF_AUTO_OFF;
>  }
> +if (s->intr_eim == ON_OFF_AUTO_AUTO && pcmc->buggy_intel_iommu_eim) {
> +s->intr_eim = ON_OFF_AUTO_ON;
> +}
>  if (s->intr_eim == ON_OFF_AUTO_AUTO) {
>  s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
>: ON_OFF_AUTO_OFF;
>  }
> -if (s->intr_eim == ON_OFF_AUTO_ON) {
> +if (s->intr_eim == ON_OFF_AUTO_ON && !pcmc->buggy_intel_iommu_eim) {
>  if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
>  error_report("intel-iommu,eim=on requires support on the KVM 
> side "
>   "(X2APIC_API, first shipped in v4.7).");

No matter how we would treat this patch, I see that we are stacking up
if clauses here. So IMHO maybe it's time to award EIM a new routine:

  int vtd_eim_detect(IntelIOMMUState *, Error **errp);

And squash all these conditions in. Then in vtd_realize():

  if (vtd_eim_detect(s, errp)) {
  return;
  }

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v12 2/2] docs: Add a generic loader explanation document

[Markus Armbruster]

Alistair Francis  writes:

> On Thu, Sep 29, 2016 at 2:24 AM, Markus Armbruster  wrote:
>> Alistair Francis  writes:
>>
>>> Signed-off-by: Alistair Francis 
>>> Reviewed-by: Peter Maydell 
>>> ---
>>> V11:
>>>  - Fix corrections
>>> V10:
>>>  - Split the data loading and PC setting
>>> V9:
>>>  - Clarify the image loading options
>>> V8:
>>>  - Improve documentation
>>> V6:
>>>  - Fixup documentation
>>> V4:
>>>  - Re-write to be more comprehensive
>>>
>>>  docs/generic-loader.txt | 81 
>>> +
>>>  1 file changed, 81 insertions(+)
>>>  create mode 100644 docs/generic-loader.txt
>>>
>>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>>> new file mode 100644
>>> index 000..d1f8ce3
>>> --- /dev/null
>>> +++ b/docs/generic-loader.txt
>>> @@ -0,0 +1,81 @@
>>> +Copyright (c) 2016 Xilinx Inc.
>>> +
>>> +This work is licensed under the terms of the GNU GPL, version 2 or later.  
>>> See
>>> +the COPYING file in the top-level directory.
>>> +
>>> +
>>> +The 'loader' device allows the user to load multiple images or values into
>>> +QEMU at startup.
>>> +
>>> +Loading Data into Memory Values
>>> +-
>>> +The loader device allows memory values to be set from the command line. 
>>> This
>>> +can be done by following the syntax below:
>>> +
>>> + -device loader,addr=,data=,data-len=
>>> +   [,data-be=][,cpu-num=]
>>> +
>>> +  - The address to store the data in.
>>> +  - The value to be written to the address. The maximum size 
>>> of
>>> +  the data is 8 bytes.
>>> +  - The length of the data in bytes. This argument must be
>>> +  included if the data argument is.
>>> +   - Set to true if the data to be stored on the guest should 
>>> be
>>> +  written as big endian data. The default is to write 
>>> little
>>> +  endian data.
>>> +   - The number of the CPU's address space where the data 
>>> should
>>> +  be loaded. If not specified the address space of the 
>>> first
>>> +  CPU is used.
>>> +
>>> +For all values both hex and decimal values are allowed. By default the 
>>> values
>>> +will be parsed as decimal. To use hex values the user should prefix the 
>>> number
>>> +with a '0x'.
>>
>> Unless you bypassed QemuOpts number parsing somehow, octal works as
>> well.  In case you did bypass: don't!  Command line consistency matters.
>> Follow-up patch reverting the bypass would be required.
>>
>> Not sure we want to document QemuOpts number syntax everywhere we
>> explain how a certain feature uses the command line.  A pointer to the
>> canonical place could be better.  Anyway, not something that needs
>> fixing before we commit.
>
> I didn't bypass it, octal should work as well. I have clarified that a
> bit in the doc.

Thanks.

>>> +
>>> +An example of loading value 0x800e to address 0xfd1a0104 is:
>>> +-device loader,addr=0xfd1a0104,data=0x800e,data-len=4
>>> +
>>> +Setting a CPU's Program Counter
>>> +-
>>> +The loader device allows the CPU's PC to be set from the command line. This
>>> +can be done by following the syntax below:
>>> +
>>> + -device loader,addr=,cpu-num=
>>> +
>>> +  - The value to use as the CPU's PC.
>>> +   - The number of the CPU whose PC should be set to the
>>> +  specified value.
>>> +
>>> +For all values both hex and decimal values are allowed. By default the 
>>> values
>>> +will be parsed as decimal. To use hex values the user should prefix the 
>>> number
>>> +with a '0x'.
>>> +
>>> +An example of setting CPU 0's PC to 0x8000 is:
>>> +-device loader,addr=0x8000,cpu-num=0
>>> +
>>> +Loading Files
>>> +-
>>> +The loader device also allows files to be loaded into memory. This can be 
>>> done
>>> +similarly to setting memory values. The syntax is shown below:
>>> +
>>> +-device 
>>> loader,file=[,addr=][,cpu-num=][,force-raw=]
>>> +
>>> +  - A file to be loaded into memory
>>> +  - The addr in memory that the file should be loaded. This 
>>> is
>>> +  ignored if you are using an ELF (unless force-raw is 
>>> true).
>>> +  This is required if you aren't loading an ELF.
>>> +   - This specifies the CPU that should be used. This is an
>>> +  optional argument and will cause the CPU's PC to be set 
>>> to
>>> +  where the image is stored or in the case of an ELF file 
>>> to
>>> +  the value in the header. This option should only be used
>>> +  for the boot image.
>>> +  This will also cause the image to be written to the 
>>> specified
>>> +  CPU's address space. If not specified, the default is 
>>> 

Re: [Qemu-devel] [PATCH 1/2] net: pcnet: check rx/tx descriptor ring length

[P J P]

  Hello Jason,

+-- On Fri, 30 Sep 2016, Jason Wang wrote --+
| On 2016年09月30日 02:57, P J P wrote:
| > The AMD PC-Net II emulator has set of control and status(CSR)
| > registers. Of these, CSR76 and CSR78 hold receive and transmit
| > descriptor ring length respectively. This ring length could range
| > from 1 to 65535. Setting ring length to zero leads to an infinite
| > loop in pcnet_rdra_addr. Add check to avoid it.
| 
| In this case, we only need to protect RCVRL I believe? (since XMTRL were not
| used).

  XMTRL is not used in this case, but could be prone to similar issues. For 
ex.

static void pcnet_transmit(PCNetState *s)
{
int count = CSR_XMTRL(s) - 1;
...
if (count--)
goto txagain;
}

If CSR_XMTRL is set to zero(0), 'count' would never reach zero and function 
would continue to jump to 'txagain'.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v2 5/5] block: keep AioContext pointer in BlockBackend

[Fam Zheng]

On Thu, 09/29 09:47, Paolo Bonzini wrote:
> 
> 
> On 29/09/2016 05:05, Fam Zheng wrote:
> > > So whether we can move a certain BB from some context to another depends
> > > on what the frontend supports, I don't think there is a generic answer
> > > we can implement here in the generic BB code. NBD for instance allows
> > > any movement; but devices probably only allow movements they have
> > > initiated themselves (e.g. dataplane will allow exactly what you
> > > describe here with that assertion, and any other device will probably
> > > not allow anything but the main loop).
> > 
> > Indeed, you make me think this should be an op blocker (that applies on 
> > whole
> > graph).
> 
> The concept of a BDS or BB's AioContext is going to disappear sooner or
> later, take this into account to decide how much effort to put into
> fixing this.

Taking one step back, the whole thing is so broken considering the node
reference. Currently, even this is possible:

-drive file=null-co://,if=virtio,id=d0 \
-drive file.file=d0,driver=raw,file.driver=raw,if=virtio

AioContext is actually a factor to make it worse. Maybe we should instead think
about some generic protection mechanism? Is this in the scope of new op
blocker?

Fam

Re: [Qemu-devel] [RFC v2] libvirt vGPU QEMU integration

[Kirti Wankhede]

On 9/29/2016 8:12 PM, Tian, Kevin wrote:
>> From: Daniel P. Berrange [mailto:berra...@redhat.com]
>> Sent: Thursday, September 29, 2016 10:39 PM
>>
>> On Thu, Sep 29, 2016 at 02:35:48PM +, Tian, Kevin wrote:
 From: Daniel P. Berrange [mailto:berra...@redhat.com]
 Sent: Thursday, September 29, 2016 4:06 PM

 On Wed, Sep 28, 2016 at 12:48:33PM -0700, Neo Jia wrote:
> On Tue, Sep 20, 2016 at 10:47:53AM +0100, Daniel P. Berrange wrote:
>> On Tue, Sep 20, 2016 at 02:05:52AM +0530, Kirti Wankhede wrote:
>>>
>>> Hi libvirt experts,
>>>
>>> Thanks for valuable input on v1 version of RFC.
>>>
>>> Quick brief, VFIO based mediated device framework provides a way to
>>> virtualize their devices without SR-IOV, like NVIDIA vGPU, Intel KVMGT
>>> and IBM's channel IO. This framework reuses VFIO APIs for all the
>>> functionalities for mediated devices which are currently being used for
>>> pass through devices. This framework introduces a set of new sysfs files
>>> for device creation and its life cycle management.
>>>
>>> Here is the summary of discussion on v1:
>>> 1. Discover mediated device:
>>> As part of physical device initialization process, vendor driver will
>>> register their physical devices, which will be used to create virtual
>>> device (mediated device, aka mdev) to the mediated framework.
>>>
>>> Vendor driver should specify mdev_supported_types in directory format.
>>> This format is class based, for example, display class directory format
>>> should be as below. We need to define such set for each class of devices
>>> which would be supported by mediated device framework.
>>>
>>>  --- mdev_destroy
>>>  --- mdev_supported_types
>>>  |-- 11
>>>  |   |-- create
>>>  |   |-- name
>>>  |   |-- fb_length
>>>  |   |-- resolution
>>>  |   |-- heads
>>>  |   |-- max_instances
>>>  |   |-- params
>>>  |   |-- requires_group
>>>  |-- 12
>>>  |   |-- create
>>>  |   |-- name
>>>  |   |-- fb_length
>>>  |   |-- resolution
>>>  |   |-- heads
>>>  |   |-- max_instances
>>>  |   |-- params
>>>  |   |-- requires_group
>>>  |-- 13
>>>  |-- create
>>>  |-- name
>>>  |-- fb_length
>>>  |-- resolution
>>>  |-- heads
>>>  |-- max_instances
>>>  |-- params
>>>  |-- requires_group
>>>
>>>
>>> In the above example directory '11' represents a type id of mdev device.
>>> 'name', 'fb_length', 'resolution', 'heads', 'max_instance' and
>>> 'requires_group' would be Read-Only files that vendor would provide to
>>> describe about that type.
>>>
>>> 'create':
>>> Write-only file. Mandatory.
>>> Accepts string to create mediated device.
>>>
>>> 'name':
>>> Read-Only file. Mandatory.
>>> Returns string, the name of that type id.
>>
>> Presumably this is a human-targetted title/description of
>> the device.
>>
>>>
>>> 'fb_length':
>>> Read-only file. Mandatory.
>>> Returns {K,M,G}, size of framebuffer.
>>>
>>> 'resolution':
>>> Read-Only file. Mandatory.
>>> Returns 'hres x vres' format. Maximum supported resolution.
>>>
>>> 'heads':
>>> Read-Only file. Mandatory.
>>> Returns integer. Number of maximum heads supported.
>>
>> None of these should be mandatory as that makes the mdev
>> useless for non-GPU devices.
>>
>> I'd expect to see a 'class' or 'type' attribute in the
>> directory whcih tells you what kind of mdev it is. A
>> valid 'class' value would be 'gpu'. The fb_length,
>> resolution, and heads parameters would only be mandatory
>> when class==gpu.
>>
>
> Hi Daniel,
>
> Here you are proposing to add a class named "gpu", which will make all 
> those gpu
> related attributes mandatory, which libvirt can allow user to better
> parse/present a particular mdev configuration?
>
> I am just wondering if there is another option that we just make all those
> attributes that a mdev device can have as optional but still meaningful to
> libvirt, so libvirt can still parse / recognize them as an class "mdev".

 'mdev' isn't a class - mdev is the name of the kernel module. The class
 refers to the broad capability of the device. class would be things
 like "gpu", "nic", "fpga" or other such things. The point of the class
 is to identify which other attributes will be considered mandatory.


>>>
>>> Thanks Daniel. This class definition makes sense to me.
>>>
>>> However I'm not sure whether we should define such common mandatory 
>>> attributes
>>> of a 'gpu' class now. Intel will go with a 2's power 

Re: [Qemu-devel] [PATCH v2 5/7] intel-iommu: add OnOffAuto intr_eim as "eim" property

[Peter Xu]

On Thu, Sep 29, 2016 at 01:23:27PM +0200, Radim Krčmář wrote:
> The default (auto) emulates the current behavior.
> 
> Signed-off-by: Radim Krčmář 
> ---
>  hw/i386/intel_iommu.c | 20 +++-
>  include/hw/i386/intel_iommu.h |  1 +
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index eb488c14625d..47141cea64f4 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2011,6 +2011,8 @@ static const MemoryRegionOps vtd_mem_ops = {
>  
>  static Property vtd_properties[] = {
>  DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
> +DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
> +ON_OFF_AUTO_AUTO),
>  DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -2367,7 +2369,11 @@ static void vtd_init(IntelIOMMUState *s)
>  s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
>  
>  if (x86_iommu->intr_supported) {
> -s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM | VTD_ECAP_MHMV;
> +s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
> +if (s->intr_eim == ON_OFF_AUTO_ON) {
> +s->ecap |= VTD_ECAP_EIM;
> +}
> +assert(s->intr_eim != ON_OFF_AUTO_AUTO);
>  }
>  
>  vtd_reset_context_cache(s);
> @@ -2466,6 +2472,18 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>  exit(1);
>  }
>  
> +if (s->intr_eim == ON_OFF_AUTO_ON && !x86_iommu->intr_supported) {
> +error_report("intel-iommu,eim=on cannot be selected without "
> + "intremap=on.");
> +exit(1);
> +}
> +if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) {
> +s->intr_eim = ON_OFF_AUTO_OFF;
> +}
> +if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> +s->intr_eim = ON_OFF_AUTO_ON;
> +}

A single if() instead of above two might be nicer:

if (s->intr_eim == ON_OFF_AUTO_AUTO) {
e->intr_eim = x86_iommu->intr_supported ?
ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
}

Otherwise good to me.

Thanks,

-- peterx

Re: [Qemu-devel] [V5 0/6] AMD IOMMU interrupt remapping

[David Kiarie]

On Tue, Sep 20, 2016 at 8:40 PM, David Kiarie  wrote:
> Hello all,
>
> This patchset mainly adds AMD IOMMU interrupt remapping logic to Qemu. Doing 
> that
> I have solved an existing issue where platform devices are not able to make 
> interrupt
> requests with and explicit SID.
>
> This series is based on the previously sent AMD IOMMU patchset and is 
> available here[1]
>
> Changes since v4
>   -Removed SID enforcement from Intel IOMMU.
>   -changed the code so that cache invalidation handler triggers with each 
> invalidation from IOMMU
>   -A few other miscallaneous fixes all suggested by Peter.

I'd very much appreciate feedback on these patches especially the part
that touches some KVM related code. This patchset enables platform
devices to make interrupt requests using an explicit SID(which is what
IOMMUs expect). The current Qemu code includes a hack that treats
IOAPIC as a special device which I believe the relevant team is not
fixing since these patches are known to be lying around.

David.

>
>
> [1] https://github.com/aslaq/qemu ir
>
> David Kiarie (6):
>   hw/msi: Allow platform devices to use explicit SID
>   hw/i386: enforce SID verification
>   hw/iommu: Prepare for AMD IOMMU interrupt remapping
>   hw/iommu: AMD IOMMU interrupt remapping
>   hw/acpi: report IOAPIC on IVRS
>   hw/iommu: share common code between IOMMUs
>
>  hw/i386/acpi-build.c  |   2 +
>  hw/i386/amd_iommu.c   | 206 
> +-
>  hw/i386/amd_iommu.h   |  80 +++
>  hw/i386/intel_iommu.c |  84 +++-
>  hw/i386/kvm/pci-assign.c  |  12 ++-
>  hw/i386/trace-events  |   7 ++
>  hw/i386/x86-iommu.c   |   8 ++
>  hw/intc/ioapic.c  |  33 --
>  hw/misc/ivshmem.c |   6 +-
>  hw/vfio/pci.c |   6 +-
>  hw/virtio/virtio-pci.c|   7 +-
>  include/hw/i386/ioapic_internal.h |   1 +
>  include/hw/i386/x86-iommu.h   |   2 +-
>  include/sysemu/kvm.h  |  25 +++--
>  kvm-all.c |  11 +-
>  kvm-stub.c|   5 +-
>  target-arm/kvm.c  |   3 +-
>  target-i386/kvm.c |  15 +--
>  target-mips/kvm.c |   3 +-
>  target-ppc/kvm.c  |   3 +-
>  target-s390x/kvm.c|   3 +-
>  21 files changed, 427 insertions(+), 95 deletions(-)
>
> --
> 2.1.4
>

[Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev()

[zhanghailiang]

find_and_check_chardev() uses 'opts' member of CharDriverState to
check if the chardev is 'socket' chardev or not, which the opts
will be NULL if We add the chardev by qmp 'chardev-add' command.

All the related info can be found in 'filename' member of CharDriverState,
For tcp socket device, it will be like 'disconnected:tcp:9.61.1.8:9004,server'
or 'tcp:9.61.1.8:9001,server <-> 9.61.1.8:50256', we can simply check it to
identify if it is a tcp socket char device.

Besides, fix this helper function to return -1 while some errors happen.

Signed-off-by: zhanghailiang 
---
 net/colo-compare.c | 54 --
 1 file changed, 8 insertions(+), 46 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 22b1da1..6693258 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -92,10 +92,6 @@ typedef struct CompareClass {
 ObjectClass parent_class;
 } CompareClass;
 
-typedef struct CompareChardevProps {
-bool is_socket;
-} CompareChardevProps;
-
 enum {
 PRIMARY_IN = 0,
 SECONDARY_IN,
@@ -564,56 +560,22 @@ static void compare_sec_rs_finalize(SocketReadState 
*sec_rs)
 }
 }
 
-static int compare_chardev_opts(void *opaque,
-const char *name, const char *value,
-Error **errp)
-{
-CompareChardevProps *props = opaque;
-
-if (strcmp(name, "backend") == 0 &&
-strcmp(value, "socket") == 0) {
-props->is_socket = true;
-return 0;
-} else if (strcmp(name, "host") == 0 ||
-  (strcmp(name, "port") == 0) ||
-  (strcmp(name, "server") == 0) ||
-  (strcmp(name, "wait") == 0) ||
-  (strcmp(name, "path") == 0)) {
-return 0;
-} else {
-error_setg(errp,
-   "COLO-compare does not support a chardev with option %s=%s",
-   name, value);
-return -1;
-}
-}
-
-/*
- * Return 0 is success.
- * Return 1 is failed.
- */
 static int find_and_check_chardev(CharDriverState **chr,
   char *chr_name,
   Error **errp)
 {
-CompareChardevProps props;
-
 *chr = qemu_chr_find(chr_name);
 if (*chr == NULL) {
 error_setg(errp, "Device '%s' not found",
chr_name);
-return 1;
+return -1;
 }
 
-memset(, 0, sizeof(props));
-if (qemu_opt_foreach((*chr)->opts, compare_chardev_opts, , errp)) {
-return 1;
-}
+if (!strstr((*chr)->filename, "tcp")) {
+error_setg(errp, "chardev \"%s\" is not a tcp socket, filename '%s'",
+   chr_name, (*chr)->filename);
+return -1;
 
-if (!props.is_socket) {
-error_setg(errp, "chardev \"%s\" is not a tcp socket",
-   chr_name);
-return 1;
 }
 return 0;
 }
@@ -660,15 +622,15 @@ static void colo_compare_complete(UserCreatable *uc, 
Error **errp)
 return;
 }
 
-if (find_and_check_chardev(>chr_pri_in, s->pri_indev, errp)) {
+if (find_and_check_chardev(>chr_pri_in, s->pri_indev, errp) < 0) {
 return;
 }
 
-if (find_and_check_chardev(>chr_sec_in, s->sec_indev, errp)) {
+if (find_and_check_chardev(>chr_sec_in, s->sec_indev, errp) < 0) {
 return;
 }
 
-if (find_and_check_chardev(>chr_out, s->outdev, errp)) {
+if (find_and_check_chardev(>chr_out, s->outdev, errp) < 0) {
 return;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] vfio: Add support for mmapping sub-page MMIO BARs

[Thorsten Kohfeldt]

(Re-post, as my mail client somehow made my previous post attach to the wrong 
thread.
I do not mean to spam y'all, but maybe my previous mail got lost in your 
filters ...
... as I have not yet seen any answer to my questions/remarks.
)

> On 2016/9/14 6:55, Alex Williamson wrote:
>
> [cc +Paolo]
>
>> On Thu, 11 Aug 2016 19:05:57 +0800
>> Yongji Xie  wrote:
>>
>> Now the kernel commit 05f0c03fbac1 ("vfio-pci: Allow to mmap
>> sub-page MMIO BARs if the mmio page is exclusive") allows VFIO
>> to mmap sub-page BARs. This is the corresponding QEMU patch.

Immediate questions first:
It seems that mentioned commit will be part of Kernel 4.8 ?
But as far as I can judge this change should also cooperate with
older/existing kernels (which then just have qemu behave as before) ?

(For my actual point of interrest related to this patch please see further 
down.)

>> With those patches applied, we could passthrough sub-page BARs
>> to guest, which can help to improve IO performance for some devices.
>>
>> In this patch, we expand MemoryRegions of these sub-page
>> MMIO BARs to PAGE_SIZE in vfio_pci_write_config(), so that
>> the BARs could be passed to KVM ioctl KVM_SET_USER_MEMORY_REGION
>> with a valid size. The expanding size will be recovered when
>> the base address of sub-page BAR is changed and not page aligned
>> any more in guest. And we also set the priority of these BARs'
>> memory regions to zero in case of overlap with BARs which share
>> the same page with sub-page BARs in guest.
>>
>> Signed-off-by: Yongji Xie 
>> ---
>> hw/vfio/common.c |3 +--
>> hw/vfio/pci.c|   76 
++
>> 2 files changed, 77 insertions(+), 2 deletions(-)
>
> Hi Yongji,
...
>> +mr = region->mem;
>> +mmap_mr = >mmaps[0].mem;
>> +memory_region_transaction_begin();
>> +if (memory_region_size(mr) < qemu_real_host_page_size) {
>> +if (!(bar_addr & ~qemu_real_host_page_mask) &&
>> +memory_region_is_mapped(mr) && region->mmaps[0].mmap) {
>> +/* Expand memory region to page size and set priority */
>> +memory_region_del_subregion(r->address_space, mr);
>> +memory_region_set_size(mr, qemu_real_host_page_size);
>> +memory_region_set_size(mmap_mr, qemu_real_host_page_size);
>> +memory_region_add_subregion_overlap(r->address_space,
>> +bar_addr, mr, 0);
>> +   }
>> +} else {
>> +/* This case would happen when guest rescan one PCI device */
>> +if (bar_addr & ~qemu_real_host_page_mask) {
>> +/* Recover the size of memory region */
>> +memory_region_set_size(mr, r->size);
>> +memory_region_set_size(mmap_mr, r->size);
>> +} else if (memory_region_is_mapped(mr)) {
>> +/* Set the priority of memory region to zero */
>> +memory_region_del_subregion(r->address_space, mr);
>> +memory_region_add_subregion_overlap(r->address_space,
>> +bar_addr, mr, 0);
>> +}
>> +}
>> +memory_region_transaction_commit();
>
> Paolo, as the reigning memory API expert, do you see any issues with
> this?  Expanding the size of a container MemoryRegion and the contained
> mmap'd subregion and manipulating priorities so that we don't interfere
> with other MemoryRegions.

Since the following qemu commit function memory_region_add_subregion_overlap()
actually has a misleading name:
http://git.qemu.org/?p=qemu.git;a=blobdiff;f=memory.c;h=ac5236b51587ee397edd177502fc20ce159f2235;hp=9daac5ea2d9a9c83533880a812760683f6e09765;hb=b61359781958759317ee6fd1a45b59be0b7dbbe1;hpb=ab0a99560857302b60053c245d1231acbd976cd4

The sole thing that memory_region_add_subregion_overlap() now actually does
differently from memory_region_add_subregion() is nothing else than setting
the region's priority to a value of callers choice.
The _default_ priority as chosen by memory_region_add_subregion() _is_ 0.

So, explicitly choosing memory_region_add_subregion_overlap(... , 0) does
nothing new.
Or does it:
Actually, _yes_, because I see Alex actually willing to discuss choice
of memory region priorities related to VFIO and mmap.
Why do I "invade" this thread ?
I would like you to consider thinking twice about selecting proper priorities
for _any_ mmap related region (i.e. also the aligned case), and here is why:
(I will also make a statement related to region expansion for alignment.)

First of all, I recently suggested a patch which can visualise what I
write about subsequently:
https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01315.html
(I would appreciate if somebody would review and thus get it merged.)

As a general remark, the sub-page mmap case does not only occur when
a 'small' BAR is encountered, it also occurs when a fully mmap-ed
page is split by a 'small' VFIO quirk.
Hi Alex, here we go 

Re: [Qemu-devel] [Qemu-ppc] [QEMU PATCH v4 0/6] migration: ensure hotplug and migration work together

[David Gibson]

On Thu, Sep 29, 2016 at 09:55:30AM -0700, Jianjun Duan wrote:
> ping

I'm sorry, this fell off my radar.  Can you please rebase and resend
your latest version.

> 
> On 06/27/2016 09:59 AM, Jianjun Duan wrote:
> > Hi all,
> >The previous patches seem to get buried deep somewhere. I am resending 
> > them
> >  without RFC tag. Comments are welcome.
> > 
> > v4: - Introduce a way to set customized instance_id in SaveStateEntry. Use 
> > it
> >   to set instance_id for DRC using its unique index to address David 
> >   Gibson's concern.
> > - Rename VMS_CSTM to VMS_LINKED based on Paolo Bonzini's suggestions.
> > - Clean up qjson stuff in put_qtailq. 
> > - Add trace for put_qtailq and get_qtailq based on David Gilbert's 
> >   suggestion.
> > 
> > It is based on David's ppc-for-2.7. Comments are welcome. Previous versions 
> > are:
> > 
> > v3: - Simplify overall design followng discussion with Paolo. No longer need
> >   metadata to migrate QTAILQ.
> > - Extend VMStateInfo instead of adding similar fields to VMStateField.
> > - Clean up macros in qemu/queue.h.
> > (link: 
> > https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg05695.html)
> > 
> > v2: - Introduce a general approach to migrate QTAILQ in qemu/queue.h.
> > - Migrate signalled field in the DRC state.
> > - Put the newly added migrating fields in subsections so that backward 
> >   migration is not broken.  
> > - Set detach_cb field right after migration so that a migrated 
> > hot-unplug
> >   event could finish its course.
> > (link: 
> > https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg04188.html)
> > 
> > v1: - Inital version.
> > (link: 
> > https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg02601.html)
> > 
> > To make guest device (PCI, CPU and memory) hotplug work together 
> > with guest migration, spapr drc state needs be transmitted in
> > migration. This patch defines the VMStateDescription struct for
> > spapr drc state to enable it.
> > 
> > To fix the potential racing between hotplug events on guest and 
> > guest migration, ccs_list and pending_events of spapr state need be 
> > transmitted in migration. This patch also takes care of it.
> > 
> > Jianjun Duan (6):
> >   migration: alternative way to set instance_id in SaveStateEntry
> >   migration: spapr_drc: defined VMStateDescription struct
> >   migration: extend VMStateInfo
> >   migration: migrate QTAILQ
> >   migration: spapr: migrate ccs_list in spapr state
> >   migration: spapr: migrate pending_events of spapr state
> > 
> >  hw/net/vmxnet3.c|  18 +++--
> >  hw/nvram/eeprom93xx.c   |   6 +-
> >  hw/nvram/fw_cfg.c   |   6 +-
> >  hw/pci/msix.c   |   6 +-
> >  hw/pci/pci.c|  12 ++--
> >  hw/pci/shpc.c   |   5 +-
> >  hw/ppc/spapr.c  |  67 ++
> >  hw/ppc/spapr_drc.c  |  69 +++
> >  hw/ppc/spapr_events.c   |  22 +++---
> >  hw/ppc/spapr_pci.c  |  22 ++
> >  hw/scsi/scsi-bus.c  |   6 +-
> >  hw/timer/twl92230.c |   6 +-
> >  hw/usb/redirect.c   |  18 +++--
> >  hw/virtio/virtio-pci.c  |   6 +-
> >  hw/virtio/virtio.c  |   6 +-
> >  include/hw/ppc/spapr.h  |   3 +-
> >  include/hw/ppc/spapr_drc.h  |   9 +++
> >  include/hw/qdev-core.h  |   6 ++
> >  include/migration/vmstate.h |  36 --
> >  include/qemu/queue.h|  32 +
> >  migration/savevm.c  |  25 +--
> >  migration/vmstate.c | 161 
> > ++--
> >  target-alpha/machine.c  |   5 +-
> >  target-arm/machine.c|  12 ++--
> >  target-i386/machine.c   |  21 --
> >  target-mips/machine.c   |  10 +--
> >  target-ppc/machine.c|  10 +--
> >  target-sparc/machine.c  |   5 +-
> >  trace-events|   4 ++
> >  29 files changed, 505 insertions(+), 109 deletions(-)
> > 
> 

-- 
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: [Qemu-devel] [RFC PATCH v0] spapr: Disable CPU unplug in TCG mode

[Bharata B Rao]

On Wed, Sep 21, 2016 at 03:31:00PM +1000, David Gibson wrote:
> On Wed, Sep 21, 2016 at 10:18:00AM +0530, Bharata B Rao wrote:
> > CPU unplug doesn't work in TCG mode currently and causes frequent system
> > freeze. In addition to other potential problems, the main problem arises
> > of out the requirement to support synchronous removal of a CPU thread.
> > The CPU thread that performs the cleanup of the unplugged CPU, kicks and
> > waits for the unplugged CPU thread to finish. This wait never finishes in
> > TCG mode when the waiting thread and the unplugged CPU thread are one and
> > the same.
> > 
> > So wait till proper MTTCG support is available before enabling
> > CPU unplug in TCG mode.
> 
> MTTCG seems like a very big hammer to fix this with.  Surely we could
> come up with a simpler interlock that would work for TCG in the
> meantime.

The following hack fixes the issue mostly. I still see some occasional
hangs which points to other potential problems.

diff --git a/cpus.c b/cpus.c
index 8ad1eb4..7dc7d09 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1526,8 +1526,13 @@ void cpu_remove(CPUState *cpu)
 void cpu_remove_sync(CPUState *cpu)
 {
 cpu_remove(cpu);
-while (cpu->created) {
-qemu_cond_wait(_cpu_cond, _global_mutex);
+if (!kvm_enabled()) {
+qemu_tcg_destroy_vcpu(cpu);
+cpu->created = false;
+} else {
+while (cpu->created) {
+qemu_cond_wait(_cpu_cond, _global_mutex);
+}
 }
 }
 
@@ -1573,6 +1578,9 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
 /* For non-MTTCG cases we share the thread */
 cpu->thread = single_tcg_cpu_thread;
 cpu->halt_cond = single_tcg_halt_cond;
+cpu->thread_id = first_cpu->thread_id;
+cpu->created = true;
+cpu->can_do_io = 1;
 }
 }
 
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index dc058e5..9558fc9 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -244,6 +244,7 @@ static void rtas_stop_self(PowerPCCPU *cpu, 
sPAPRMachineState *spapr,
 CPUPPCState *env = >env;
 
 cs->halted = 1;
+cs->stop = true;
 qemu_cpu_kick(cs);
 /*
  * While stopping a CPU, the guest calls H_CPPR which

This is however on Alex's MTTCG tree, I need to figure out which are
the fixes that are relavent from Alex's tree to get CPU unplug working
in TCG mode.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v7 3/4] vfio iommu: Add support for mediated devices

[Jike Song]

On 09/30/2016 10:58 AM, Jike Song wrote:
> On 09/29/2016 11:06 PM, Kirti Wankhede wrote:
>>
>>
>> On 9/29/2016 7:47 AM, Jike Song wrote:
>>> +Guangrong
>>>
>>> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:
>>
>> ...
>>
 +static long vfio_iommu_type1_pin_pages(void *iommu_data,
 + unsigned long *user_pfn,
 + long npage, int prot,
 + unsigned long *phys_pfn)
 +{
 +  struct vfio_iommu *iommu = iommu_data;
 +  struct vfio_domain *domain;
 +  int i, j, ret;
 +  long retpage;
 +  unsigned long remote_vaddr;
 +  unsigned long *pfn = phys_pfn;
 +  struct vfio_dma *dma;
 +  bool do_accounting = false;
 +
 +  if (!iommu || !user_pfn || !phys_pfn)
 +  return -EINVAL;
 +
 +  mutex_lock(>lock);
 +
 +  if (!iommu->local_domain) {
 +  ret = -EINVAL;
 +  goto pin_done;
 +  }
 +
 +  domain = iommu->local_domain;
 +
 +  /*
 +   * If iommu capable domain exist in the container then all pages are
 +   * already pinned and accounted. Accouting should be done if there is no
 +   * iommu capable domain in the container.
 +   */
 +  do_accounting = !IS_IOMMU_CAPABLE_DOMAIN_IN_CONTAINER(iommu);
 +
 +  for (i = 0; i < npage; i++) {
 +  struct vfio_pfn *p;
 +  dma_addr_t iova;
 +
 +  iova = user_pfn[i] << PAGE_SHIFT;
 +
 +  dma = vfio_find_dma(iommu, iova, 0);
 +  if (!dma) {
 +  ret = -EINVAL;
 +  goto pin_unwind;
 +  }
 +
 +  remote_vaddr = dma->vaddr + iova - dma->iova;
 +
 +  retpage = __vfio_pin_pages_local(domain, remote_vaddr, prot,
 +   [i], do_accounting);
>>>
>>> Hi Kirti,
>>>
>>> Here you call __vfio_pin_pages_local() > vaddr_get_pfn() > GUP regardless
>>> whether the vaddr already pinned or not. That probably means, if the caller 
>>> calls vfio_pin_pages() with a GPA for multiple times, you get memory leaks.
>>>
>>> GUP always increases the page refcnt.
>>>
>>> FWIW, I would like to have the pfn_list_lock implemented with key == iova,
>>> so you can always try to find the PFN for a given iova, and pin it only if
>>> not found.
>>>
>>
>> I didn't get how there would be a memory leak.
>>
>> Right, GUP increases refcnt, so if vfio_pin_pages() is called for
>> multiple types for same GPA, refcnt would be incremented. In
>> vfio_iommu_type1_pin_pages() pinned pages list is maintained with
>> ref_count. If pfn is already in list, ref_count is incremented and same
>> is used while unpining pages.
>>
> 
> Let's have a close look at vfio_unpin_pfn:
> 
>   static int vfio_unpin_pfn(struct vfio_domain *domain,
> struct vfio_pfn *vpfn, bool do_accounting)
>   {
>   __vfio_unpin_pages_for_mdev(domain, vpfn->pfn, vpfn->prot,
>   do_accounting);
> 
>   if (atomic_dec_and_test(>ref_count))
>   vfio_remove_from_pfn_list(domain, vpfn);
> 
>   return 1;
>   }
> 
> Here you didn't call __vfio_unpin_pages_for_mdev -- thereby put_page -- for
> vpfn->ref_count times. If page->_refcount increased by GUP for (N) times, here
> you only set it back to (N-1).
> 

What's more, since all pinned {iova, pfni} already saved, it's better to
consult it before calling GUP, which will get_page() unconditionally.

--
Thanks,
Jike

Re: [Qemu-devel] [PATCH 2/2] net: pcnet: fix source formatting and indentation

[Jason Wang]

On 2016年09月30日 02:57, P J P wrote:

From: Prasad J Pandit 

Fix indentations and source format at few places. Add braces
around few 'if' and 'while' statements.

Signed-off-by: Prasad J Pandit 


Thanks but there're still several other places that needs to be fixed 
were reported by robot.


Please fix them too.

Re: [Qemu-devel] [PATCH 1/2] net: pcnet: check rx/tx descriptor ring length

[Jason Wang]

On 2016年09月30日 02:57, P J P wrote:

From: Prasad J Pandit 

The AMD PC-Net II emulator has set of control and status(CSR)
registers. Of these, CSR76 and CSR78 hold receive and transmit
descriptor ring length respectively. This ring length could range
from 1 to 65535. Setting ring length to zero leads to an infinite
loop in pcnet_rdra_addr. Add check to avoid it.


In this case, we only need to protect RCVRL I believe? (since XMTRL were 
not used).




Reported-by: Li Qiang 
Signed-off-by: Prasad J Pandit 
---
  hw/net/pcnet.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index 198a01f..3078de8 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -1429,8 +1429,11 @@ static void pcnet_csr_writew(PCNetState *s, uint32_t 
rap, uint32_t new_value)
  case 47: /* POLLINT */
  case 72:
  case 74:
+break;
  case 76: /* RCVRL */
  case 78: /* XMTRL */
+val = (val > 0) ? val : 512;
+break;
  case 112:
 if (CSR_STOP(s) || CSR_SPND(s))
 break;

Re: [Qemu-devel] [PATCH v7 3/4] vfio iommu: Add support for mediated devices

[Jike Song]

On 09/29/2016 11:06 PM, Kirti Wankhede wrote:
> 
> 
> On 9/29/2016 7:47 AM, Jike Song wrote:
>> +Guangrong
>>
>> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:
> 
> ...
> 
>>> +static long vfio_iommu_type1_pin_pages(void *iommu_data,
>>> +  unsigned long *user_pfn,
>>> +  long npage, int prot,
>>> +  unsigned long *phys_pfn)
>>> +{
>>> +   struct vfio_iommu *iommu = iommu_data;
>>> +   struct vfio_domain *domain;
>>> +   int i, j, ret;
>>> +   long retpage;
>>> +   unsigned long remote_vaddr;
>>> +   unsigned long *pfn = phys_pfn;
>>> +   struct vfio_dma *dma;
>>> +   bool do_accounting = false;
>>> +
>>> +   if (!iommu || !user_pfn || !phys_pfn)
>>> +   return -EINVAL;
>>> +
>>> +   mutex_lock(>lock);
>>> +
>>> +   if (!iommu->local_domain) {
>>> +   ret = -EINVAL;
>>> +   goto pin_done;
>>> +   }
>>> +
>>> +   domain = iommu->local_domain;
>>> +
>>> +   /*
>>> +* If iommu capable domain exist in the container then all pages are
>>> +* already pinned and accounted. Accouting should be done if there is no
>>> +* iommu capable domain in the container.
>>> +*/
>>> +   do_accounting = !IS_IOMMU_CAPABLE_DOMAIN_IN_CONTAINER(iommu);
>>> +
>>> +   for (i = 0; i < npage; i++) {
>>> +   struct vfio_pfn *p;
>>> +   dma_addr_t iova;
>>> +
>>> +   iova = user_pfn[i] << PAGE_SHIFT;
>>> +
>>> +   dma = vfio_find_dma(iommu, iova, 0);
>>> +   if (!dma) {
>>> +   ret = -EINVAL;
>>> +   goto pin_unwind;
>>> +   }
>>> +
>>> +   remote_vaddr = dma->vaddr + iova - dma->iova;
>>> +
>>> +   retpage = __vfio_pin_pages_local(domain, remote_vaddr, prot,
>>> +[i], do_accounting);
>>
>> Hi Kirti,
>>
>> Here you call __vfio_pin_pages_local() > vaddr_get_pfn() > GUP regardless
>> whether the vaddr already pinned or not. That probably means, if the caller 
>> calls vfio_pin_pages() with a GPA for multiple times, you get memory leaks.
>>
>> GUP always increases the page refcnt.
>>
>> FWIW, I would like to have the pfn_list_lock implemented with key == iova,
>> so you can always try to find the PFN for a given iova, and pin it only if
>> not found.
>>
> 
> I didn't get how there would be a memory leak.
> 
> Right, GUP increases refcnt, so if vfio_pin_pages() is called for
> multiple types for same GPA, refcnt would be incremented. In
> vfio_iommu_type1_pin_pages() pinned pages list is maintained with
> ref_count. If pfn is already in list, ref_count is incremented and same
> is used while unpining pages.
> 

Let's have a close look at vfio_unpin_pfn:

static int vfio_unpin_pfn(struct vfio_domain *domain,
  struct vfio_pfn *vpfn, bool do_accounting)
{
__vfio_unpin_pages_for_mdev(domain, vpfn->pfn, vpfn->prot,
do_accounting);

if (atomic_dec_and_test(>ref_count))
vfio_remove_from_pfn_list(domain, vpfn);

return 1;
}

Here you didn't call __vfio_unpin_pages_for_mdev -- thereby put_page -- for
vpfn->ref_count times. If page->_refcount increased by GUP for (N) times, here
you only set it back to (N-1).

--
Thanks,
Jike

Re: [Qemu-devel] [PATCH] tap-bsd: OpenBSD uses tap(4) now

[Jason Wang]

On 2016年09月29日 12:55, Thomas Huth wrote:

On 29.09.2016 03:41, Jason Wang wrote:


On 2016年09月26日 18:35, Thomas Huth wrote:

On 26.09.2016 03:58, Brad Smith wrote:

Update the tap-bsd code now that OpenBSD uses tap(4).

Signed-off-by: Brad Smith 


diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index c506ac3..8d0f049 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -55,11 +55,7 @@ int tap_open(char *ifname, int ifname_size, int
*vnet_hdr,
   if (*ifname) {
   snprintf(dname, sizeof dname, "/dev/%s", ifname);
   } else {
-#if defined(__OpenBSD__)
-snprintf(dname, sizeof dname, "/dev/tun%d", i);
-#else
   snprintf(dname, sizeof dname, "/dev/tap%d", i);
-#endif
   }
   TFR(fd = open(dname, O_RDWR));
   if (fd >= 0) {

Looking at http://man.openbsd.org/tap.4 it seems like OpenBSD is indeed
supporting /dev/tap nowadays, so:

A question is what happens if we are using old version of OpenBSD which
does not have tap?

You can still specify the ifname manually, can't you?

  Thomas



Yes, but the codes were for cases that ifname were not given.

Thanks

Re: [Qemu-devel] summary of current vfio mdev upstreaming status

[Jike Song]

On 09/29/2016 06:58 PM, Kirti Wankhede wrote:
> 
> 
> On 9/29/2016 2:47 PM, Neo Jia wrote:
>> On Thu, Sep 29, 2016 at 04:55:39PM +0800, Jike Song wrote:
>>> Hi all,
>>>
>>> In order to have a clear understanding about the VFIO mdev upstreaming
>>> status, I'd like to summarize it. Please share your opinions on this,
>>> and correct my misunderstandings.
>>>
>>> The whole vfio mdev series can be logically divided into several parts,
>>> they work together to provide the mdev support.
>>
> 
> Thanks Jike for summarizing. We already have separate patch for each of
> these logical parts. I had maintained patch sequence in incremental
> depending order.
> 
>> Hi Jike,
>>
>> Thanks for summarizing this, but I will defer to Kirti to comment on the 
>> actual
>> upstream status of her patches, couples things to note though:
>>
>> 1) iommu type1 patches have been extensively reviewed by Alex already and we
>> have one action item left to implement which is already queued up in v8 
>> patchset.
>>
> 
> That's right Neo.
> 

I'm talking about v7. Sure before that Alex gave full reviews..

>> 2) regarding the sysfs interface and libvirt discussion, I would like to hear
>> what kind of attributes Intel folks are having so far as Daniel is
>> asking about adding a class "gpu" which will pull several attributes as 
>> mandatory.
>>

As Kevin said, no. 

>> Thanks,
>> Neo
>>
>>>
>>>
>>>
>>> PART 1: mdev core driver
>>>
>>> [task]
>>> -   the mdev bus/device support
>>> -   the utilities of mdev lifecycle management
>>> -   the physical device register/unregister interfaces
>>>
>>> [status]
>>> -   basically agreed by community
>>>
>>>
>>> PART 2: vfio bus driver for mdev
>>>
>>> [task]
>>> -   interfaces with vendor drivers
>>> -   the vfio bus implementation
>>>
>>> [status]
>>>
>>> -   basically agreed by community
>>>
> 
> I'm working on v8 version for above patches based on previous discussions.
> 
>>>
>>> PART 3: iommu support for mdev
>>>
>>> [task]
>>> -   iommu support for mdev
>>>
>>> [status]
>>> -   Kirti's v7 implementation, not yet fully reviewed
>>>
>>>
>>> PART 4: sysfs interfaces for mdev
>>>
>>> [task]
>>> -   define the hierarchy of minimal sysfs directories/files
>>> -   check the validity from vendor drivers, init/de-init 
>>> them
>>> [status]
>>> -   interfaces are in discussion
>>>
>>>
> 
> From coding perspective, this is part of mdev core module. I think we
> can't completely separate this part from mdev core module while coding
> it. Yes, this interface is still in discussion and we need to settle
> down on that soon.
> 

I Still think it's possible to separate them, but hey, looking forward to
your implementation :)

>>> PART 6: Documentation
>>>
>>> [task]
>>> -   clearly document the architecture and interfaces
>>> -   coding example for vendor drivers
>>>
>>> [status]
>>> -   N/A
>>>
> 
> I had tried to maintain the document as per changes going on in above
> patches from v6 onward and will continue to update it for each version
> accordingly.
> 
> I had sent out patch with sample driver few hours back wrt v7 patchset.
> And henceforth I'll keep on updating sample driver as per changes in
> mdev modules and add it in my patch series.

Good to know that.

> 
> Thanks,
> Kirti
> 
--
Thanks,
Jike

[Qemu-devel] [PATCH] net/filter-mirror: Fix mirror initial check typo

[Zhang Chen]

Signed-off-by: Zhang Chen 
---
 net/filter-mirror.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index 35df374..0ee58d9 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -198,7 +198,7 @@ static void filter_mirror_setup(NetFilterState *nf, Error 
**errp)
 MirrorState *s = FILTER_MIRROR(nf);
 
 if (!s->outdev) {
-error_setg(errp, "filter filter mirror needs 'outdev' "
+error_setg(errp, "filter mirror needs 'outdev' "
"property set");
 return;
 }
@@ -315,7 +315,7 @@ filter_mirror_set_outdev(Object *obj, const char *value, 
Error **errp)
 g_free(s->outdev);
 s->outdev = g_strdup(value);
 if (!s->outdev) {
-error_setg(errp, "filter filter mirror needs 'outdev' "
+error_setg(errp, "filter mirror needs 'outdev' "
"property set");
 return;
 }
-- 
2.7.4

Re: [Qemu-devel] [Qemu-block] [PATCH v3] block: Turn on "unmap" in active commit

[Fam Zheng]

On Thu, 09/29 13:41, Stefan Hajnoczi wrote:
> On Tue, Sep 27, 2016 at 07:14:52PM +0800, Fam Zheng wrote:
> > We already specified BDRV_O_UNMAP when opening images in 'qemu-img
> > commit', but didn't turn on the "unmap" in the active commit job. This
> > patch fixes that so that zeroed clusters in top image can be discarded
> > which is desired in the virt-sparsify use case, where a temporary
> > overlay is created and fstrim'ed before commiting back, to free space in
> > the original image.
> > 
> > This also enables it for block-commit.
> > 
> > Signed-off-by: Fam Zheng 
> > ---
> > v3: Change the right parameter.
> > v2: Add "unmap" to block-commit as well. [Kevin]
> > ---
> >  block/mirror.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index f9d1fec..8847ec5 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -1042,7 +1042,7 @@ void commit_active_start(const char *job_id, 
> > BlockDriverState *bs,
> >  
> >  mirror_start_job(job_id, bs, base, NULL, speed, 0, 0,
> >   MIRROR_LEAVE_BACKING_CHAIN,
> > - on_error, on_error, false, cb, opaque, _err,
> > + on_error, on_error, true, cb, opaque, _err,
> >   _active_job_driver, false, base, 
> > auto_complete);
> >  if (local_err) {
> >  error_propagate(errp, local_err);
> 
> Why is unmap an option at all?
> 
> What's wrong with using BDRV_REQ_MAY_UNMAP on all
> blk_aio_pwrite_zeroes() calls?

Because unmap is an QMP option of drive-backup. I think in the drive-mirror
context, it mitigates the limitation that we have no control over target's
BDRV_O_UNMAP (always inherited from source).

Fam

Re: [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP

[Fam Zheng]

On Thu, 09/29 12:39, Kevin Wolf wrote:
> Am 29.09.2016 um 11:55 hat Fam Zheng geschrieben:
> > On Thu, 09/29 11:29, Kevin Wolf wrote:
> > > Am 28.09.2016 um 09:04 hat Fam Zheng geschrieben:
> > > > Handling this is similar to what is done to the L2 entry in the case of
> > > > compressed clusters.
> > > > 
> > > > Signed-off-by: Fam Zheng 
> > > > ---
> > > >  block/qcow2-cluster.c | 9 +
> > > >  block/qcow2.c | 3 ++-
> > > >  block/qcow2.h | 3 ++-
> > > >  3 files changed, 9 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > > > index 61d1ffd..928c1e2 100644
> > > > --- a/block/qcow2-cluster.c
> > > > +++ b/block/qcow2-cluster.c
> > > > @@ -1558,7 +1558,7 @@ fail:
> > > >   * clusters.
> > > >   */
> > > >  static int zero_single_l2(BlockDriverState *bs, uint64_t offset,
> > > > -  uint64_t nb_clusters)
> > > > +  uint64_t nb_clusters, int flags)
> > > >  {
> > > >  BDRVQcow2State *s = bs->opaque;
> > > >  uint64_t *l2_table;
> > > > @@ -1582,7 +1582,7 @@ static int zero_single_l2(BlockDriverState *bs, 
> > > > uint64_t offset,
> > > >  
> > > >  /* Update L2 entries */
> > > >  qcow2_cache_entry_mark_dirty(bs, s->l2_table_cache, l2_table);
> > > > -if (old_offset & QCOW_OFLAG_COMPRESSED) {
> > > > +if (old_offset & QCOW_OFLAG_COMPRESSED || flags & 
> > > > BDRV_REQ_MAY_UNMAP) {
> > > >  l2_table[l2_index + i] = cpu_to_be64(QCOW_OFLAG_ZERO);
> > > >  qcow2_free_any_clusters(bs, old_offset, 1, 
> > > > QCOW2_DISCARD_REQUEST);
> > > >  } else {
> > > 
> > > I don't think we should always do this for BDRV_REQ_MAY_UNMAP. The user
> > > may want to keep the image fully allocated even if the guest (or block
> > > job etc.) thinks this can be discarded.
> > > 
> > > When we discussed this in another email thread (?), I think I suggested
> > > checking whether the image was opened with pass-discard-request=on. If
> > > so, go ahead and deallocate the cluster, otherwise keep it allocated.
> > > 
> > > In those cases where pass-discard-request=off, it doesn't make a lot of
> > > sense to deallocate the cluster anyway because it won't shrink the file
> > > size.
> > 
> > I think this patch does what you mean:
> > 
> > $ strace -f -e fallocate qemu-io \
> > -c 'open -o pass-discard-request=on /var/tmp/test' \
> > -c 'write 0 1M' \
> > -c 'write -u -z 0 1M' \
> > 2>&1 >/dev/null | \
> > grep fallocate
> > [pid 17548] fallocate(17, FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE, 
> > 327680, 1048576) = 0
> > $ strace -f -e fallocate qemu-io \
> > -c 'open -o pass-discard-request=off /var/tmp/test' \
> > -c 'write 0 1M' \
> > -c 'write -u -z 0 1M' \
> > 2>&1 >/dev/null | \
> > grep fallocate
> > 
> > Because there is another check of pass-discard-request value in
> > update_refcount:
> > 
> > if (refcount == 0 && s->discard_passthrough[type]) {
> > update_refcount_discard(bs, cluster_offset, s->cluster_size);
> > }
> 
> What I mean is that in the second case, you're still uselessly
> deallocating the cluster on the qcow2 level while you can't reclaim it
> on the filesystem level. So it would be better to leave it allocated in
> qcow2, too, so that you don't get an expensive reallocation the next
> time you write to it.
> 

Like this?

---

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 928c1e2..3d3cea4 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1582,7 +1582,9 @@ static int zero_single_l2(BlockDriverState *bs, uint64_t 
offset,

 /* Update L2 entries */
 qcow2_cache_entry_mark_dirty(bs, s->l2_table_cache, l2_table);
-if (old_offset & QCOW_OFLAG_COMPRESSED || flags & BDRV_REQ_MAY_UNMAP) {
+if (old_offset & QCOW_OFLAG_COMPRESSED ||
+((flags & BDRV_REQ_MAY_UNMAP) &&
+ s->discard_passthrough[QCOW2_DISCARD_REQUEST])) {
 l2_table[l2_index + i] = cpu_to_be64(QCOW_OFLAG_ZERO);
 qcow2_free_any_clusters(bs, old_offset, 1, QCOW2_DISCARD_REQUEST);
 } else {

Re: [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel

[Peter Maydell]

On 22 September 2016 at 23:33, Michael Olbrich  wrote:
> On Thu, Sep 22, 2016 at 05:23:17PM +0100, Peter Maydell wrote:
>> On 10 September 2016 at 16:07, Michael Olbrich  
>> wrote:
>> > diff --git a/vl.c b/vl.c
>> > index ee557a1d3f8a..bbea51e0ce7d 100644
>> > --- a/vl.c
>> > +++ b/vl.c
>> > @@ -4335,11 +4335,6 @@ int main(int argc, char **argv, char **envp)
>> >  exit(1);
>> >  }
>> >
>> > -if (!linux_boot && qemu_opt_get(machine_opts, "dtb")) {
>> > -error_report("-dtb only allowed with -kernel option");
>> > -exit(1);
>> > -}
>> > -
>>
>> I can see why you want this change, but what worries me a little
>> is that this is changing the behaviour of -dtb for all QEMU
>> target architectures, not just ARM (they no longer get a helpful
>> message on user error). I'm not sure how to address that, though.
>
> Would a 'if !arm' be possible or useful here?

It's not quite that simple :-)

I think we have two choices:
(1) just go ahead and remove the error-check, on the basis that:
 * for some boards -dtb is useful even without -kernel
 * -dtb might be ignored even with -kernel if the specified
   kernel isn't a DTB-aware kernel, but we ignore that
 * -dtb is ignored even with -kernel for target archs/boards
   which don't support or use DTB, and we don't warn about that
 * we don't warn about -kernel being useless for target boards
   that don't pay any attention to it
(2) add some kind of field to MachineClass indicating whether
   the machine can handle dtb files with/without a kernel
   (and perhaps also whether the machine supports -kernel at all),
   use that to gate the warning messages, and update all the
   machines to correctly indicate what they can or can't handle.
   This would let us give warning messages when the user asks
   for something we're going to ignore (including letting us
   fix up some of the cases we don't currently deal with as
   enumerated above), but it would be a fair chunk of effort
   for a fairly small user-friendliness gain

Thinking about it more, I'm inclining towards the simpler
option (1). Paolo, do you have an opinion here ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v12 0/2] Add a generic loader

[Fam Zheng]

On Thu, 09/29 11:25, Markus Armbruster wrote:
> no-re...@ec2-52-6-146-230.compute-1.amazonaws.com writes:
> 
> > Hi,
> >
> > Your series failed automatic build test. Please find the testing commands 
> > and
> > their output below. If you have docker installed, you can probably 
> > reproduce it
> > locally.
> >
> > Type: series
> > Message-id: cover.1475102513.git.alistair.fran...@xilinx.com
> > Subject: [Qemu-devel] [PATCH v12 0/2]  Add a generic loader
> >
> > === TEST SCRIPT BEGIN ===
> > #!/bin/bash
> > set -e
> > git submodule update --init dtc
> > # Let docker tests dump environment info
> > export SHOW_ENV=1
> > make J=8 docker-test-quick@centos6
> > make J=8 docker-test-mingw@fedora
> > === TEST SCRIPT END ===
> >
> > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> > Switched to a new branch 'test'
> > e1a89b7 docs: Add a generic loader explanation document
> > 1826cff generic-loader: Add a generic loader
> >
> > === OUTPUT BEGIN ===
> > Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 
> > 'dtc'
> > Cloning into 'dtc'...
> > Submodule path 'dtc': checked out '65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf'
> >   BUILD centos6
> > === OUTPUT END ===
> >
> > Abort: command timeout (>3600 seconds)
> 
> Patchew hiccup or a real problem?

Likely a docker hiccup. The laste line being "BUILD centos6" means the "make
docker-test-quick@centos6" is hung. I don't know much more either. :(

Fam

Re: [Qemu-devel] [PATCH v1 1/1] cadence_gem: Fix priority queue out of bounds access

[Peter Maydell]

On 29 September 2016 at 17:42, Alistair Francis
 wrote:
> On Thu, Sep 29, 2016 at 5:04 PM, Peter Maydell  
> wrote:
>> On 26 September 2016 at 10:49, Alistair Francis
>>  wrote:
>>> There was an error with some of the register implementation assuming
>>> there are 16 priority queues supported when the IP only supports 8. This
>>> patch corrects the registers to only support 8 queues.
>>>
>>> Signed-off-by: Alistair Francis 
>>> Reported-by: Paolo Bonzini 
>>> ---
>>> Thanks to Paolo for pointing this out.
>>>
>>>  hw/net/cadence_gem.c | 22 --
>>>  1 file changed, 4 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>>> index 8618e7a..7915732 100644
>>> --- a/hw/net/cadence_gem.c
>>> +++ b/hw/net/cadence_gem.c
>>> @@ -147,25 +147,19 @@
>>>  #define GEM_INT_Q1_MASK (0x0640 / 4)
>>>
>>>  #define GEM_TRANSMIT_Q1_PTR (0x0440 / 4)
>>> -#define GEM_TRANSMIT_Q15_PTR(GEM_TRANSMIT_Q1_PTR + 14)
>>> +#define GEM_TRANSMIT_Q7_PTR (GEM_TRANSMIT_Q1_PTR + 6)
>>
>> 8 queues but they're numbered one to ... seven ??
>
> Yeah, I double checked the user guide and it is 1 to 7.
>
> I guess queue 0 uses the standard registers.

OK. The arithmetic of the array size vs the number of
registers looks OK, anyway.

Applied to target-arm.next.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v13 0/2] Add a generic loader

[Peter Maydell]

On 29 September 2016 at 17:25, Alistair Francis
 wrote:
> This work is based on the original work by Li Guang with extra
> features added by Peter C and myself.
>
> The idea of this loader is to allow the user to load multiple images
> or values into QEMU at startup.
>
> Memory values can be loaded like this: -device 
> loader,addr=0xfd1a0104,data=0x800e,data-len=4
>
> Images can be loaded like this: -device loader,file=./images/u-boot.elf,cpu=0
>
> This can be useful and we use it a lot in Xilinx to load multiple images
> into a machine at creation (ATF, Kernel and DTB for example).
>
> Tested with the latest Xilinx ZynqMP machine, if I enable EL3 and EL2 I can
> boot ATF through to u-boot using the loader to load the images.
>
> It can also be used to set registers.
>
> This patch series makes the load_elf() function more generic by not
> requiring an architecture. It also adds new functions load_elf_as(),
> load_uimage_as and load_image_targphys_as which allows custom
> AddressSpaces when loading images.
>
> V12:
>  - All patches have been reviewed
>  - Most patches have been merged
>  - The commit message of the actual device patch has been updated to
>justify why it is a device.
>
>
> Alistair Francis (2):
>   generic-loader: Add a generic loader
>   docs: Add a generic loader explanation document

Applied to target-arm.next, thanks.

-- PMM

Re: [Qemu-devel] [Bug 1625987] Re: target-arm/translate-a64.c:2028: possible coding error ?

[Peter Maydell]

On 22 September 2016 at 02:53, Peter Maydell  wrote:
> This is clearly a bug, but your suggested change won't deal with the
> problem, which is that we're trying to set a bool so the ? 32 : 64
> construct is just wrong.

> Bug description:
>   target-arm/translate-a64.c:2028:37: warning: ?: using integer
>   constants in boolean context [-Wint-in-bool-context]
>
>   Source code is
>
>   bool iss_sf = opc == 0 ? 32 : 64;

Edgar, did you want to look at a fix for this? It was introduced
in your commit aaa1f954d4 adding syndrome info for loads and stores.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] tareget-arm: Handle tagged addresses when loading PC

[Peter Maydell]

On 16 September 2016 at 10:34, Thomas Hanson  wrote:
> If tagged addresses are enabled, then addresses being loaded into the
> PC must be cleaned up by overwriting the tag bits with either all 0's
> or all 1's as specified in the ARM ARM spec.  The decision process is
> dependent on whether the code will be running in EL0/1 or in EL2/3 and
> is controlled by a combination of Top Byte Ignored (TBI) bits in the
> TCR and the value of bit 55 in the address being loaded.
>
> TBI values are extracted from the appropriate TCR and made available
> to TCG code generation routines by inserting them into the TB flags
> field and then transferring them to DisasContext structure in
> gen_intermediate_code_a64().
>
> New function gen_a64_set_pc_reg() encapsulates the logic required to
> determine whether clean up of the tag byte is required and then
> generating the code to correctly load the PC.
>
> In addition to those instruction which can directly load a tagged
> address into the PC, there are others which increment or add a value to
> the PC.  If 56 bit addressing is used, these instructions can cause an
> arithmetic roll-over into the tag bits.  The ARM ARM specification for
> handling tagged addresses requires that these cases also be addressed
> by cleaning up the tag field.  This work has been deferred because
> there is currently no CPU model available for testing with 56 bit
> addresses.

These changes are OK (other than the comments I've made on the
patches), but do not cover all the cases where values can be
loaded into the PC and may need to be cleansed of their tags.

In particular:
 * on exception entry to AArch64 we may need to clean a tag out of
   the vector table base address register VBAR_ELx
   (in QEMU this would be in arm_cpu_do_interrupt_aarch64())
 * on exception return to AArch64 we may need to clean a tag out of
   the return address we got from ELR_ELx
   (in QEMU, in the exception_return helper)

Note that D4.1.1 of the ARM ARM describes a potential relaxation
of the requirement that tag bits not be propagated into the PC
in the case of an illegal exception return; I recommend not
taking advantage of that relaxation unless it really does fall
out of the implementation much more trivially that way.

Watch out that you use the TBI bits for the destination EL in
each case, not the EL you start in...

thanks
-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/3] tests: use qtest_pc_boot()/qtest_pc_shutdown() in virtio tests

[David Gibson]

On Thu, Sep 29, 2016 at 07:15:05PM +0200, Laurent Vivier wrote:
> Signed-off-by: Laurent Vivier 

This could do with a commit message, even if it's just to say that
this is supposed to be a refactor without behavioural change.

> ---
>  tests/virtio-9p-test.c   |  53 
>  tests/virtio-blk-test.c  | 154 
> +--
>  tests/virtio-net-test.c  |  40 +---
>  tests/virtio-scsi-test.c |  70 ++---
>  4 files changed, 140 insertions(+), 177 deletions(-)
> 
> diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
> index e8b2196..28d7f5b 100644
> --- a/tests/virtio-9p-test.c
> +++ b/tests/virtio-9p-test.c
> @@ -10,62 +10,57 @@
>  #include "qemu/osdep.h"
>  #include "libqtest.h"
>  #include "qemu-common.h"
> -#include "libqos/pci-pc.h"
> +#include "libqos/libqos-pc.h"
>  #include "libqos/virtio.h"
>  #include "libqos/virtio-pci.h"
> -#include "libqos/malloc.h"
> -#include "libqos/malloc-pc.h"
>  #include "standard-headers/linux/virtio_ids.h"
>  #include "standard-headers/linux/virtio_pci.h"
>  
>  static const char mount_tag[] = "qtest";
>  static char *test_share;
>  
> -static void qvirtio_9p_start(void)
> -{
> -char *args;
>  
> +static QOSState *qvirtio_9p_start(void)
> +{
>  test_share = g_strdup("/tmp/qtest.XX");
>  g_assert_nonnull(mkdtemp(test_share));
> +const char *cmd = "-fsdev local,id=fsdev0,security_model=none,path=%s "
> +  "-device virtio-9p-pci,fsdev=fsdev0,mount_tag=%s";
>  
> -args = g_strdup_printf("-fsdev 
> local,id=fsdev0,security_model=none,path=%s "
> -   "-device virtio-9p-pci,fsdev=fsdev0,mount_tag=%s",
> -   test_share, mount_tag);
> -
> -qtest_start(args);
> -g_free(args);
> +return qtest_pc_boot(cmd, test_share, mount_tag);
>  }
>  
> -static void qvirtio_9p_stop(void)
> +static void qvirtio_9p_stop(QOSState *qs)
>  {
> -qtest_end();
> +qtest_pc_shutdown(qs);

Shouldn't this be the generic shutdown call you added in the other series?

>  rmdir(test_share);
>  g_free(test_share);
>  }
>  
>  static void pci_nop(void)
>  {
> -qvirtio_9p_start();
> -qvirtio_9p_stop();
> +QOSState *qs;
> +
> +qs = qvirtio_9p_start();
> +g_assert(qs);
> +qvirtio_9p_stop(qs);
>  }
>  
>  typedef struct {
>  QVirtioDevice *dev;
> -QGuestAllocator *alloc;
> -QPCIBus *bus;
> +QOSState *qs;
>  QVirtQueue *vq;
>  } QVirtIO9P;
>  
> -static QVirtIO9P *qvirtio_9p_pci_init(void)
> +static QVirtIO9P *qvirtio_9p_pci_init(QOSState *qs)
>  {
>  QVirtIO9P *v9p;
>  QVirtioPCIDevice *dev;
>  
>  v9p = g_new0(QVirtIO9P, 1);
> -v9p->alloc = pc_alloc_init();
> -v9p->bus = qpci_init_pc(NULL);
>  
> -dev = qvirtio_pci_device_find(v9p->bus, VIRTIO_ID_9P);
> +v9p->qs = qs;
> +dev = qvirtio_pci_device_find(v9p->qs->pcibus, VIRTIO_ID_9P);
>  g_assert_nonnull(dev);
>  g_assert_cmphex(dev->vdev.device_type, ==, VIRTIO_ID_9P);
>  v9p->dev = (QVirtioDevice *) dev;
> @@ -75,17 +70,15 @@ static QVirtIO9P *qvirtio_9p_pci_init(void)
>  qvirtio_set_acknowledge(_pci, v9p->dev);
>  qvirtio_set_driver(_pci, v9p->dev);
>  
> -v9p->vq = qvirtqueue_setup(_pci, v9p->dev, v9p->alloc, 0);
> +v9p->vq = qvirtqueue_setup(_pci, v9p->dev, v9p->qs->alloc, 0);
>  return v9p;
>  }
>  
>  static void qvirtio_9p_pci_free(QVirtIO9P *v9p)
>  {
> -qvirtqueue_cleanup(_pci, v9p->vq, v9p->alloc);
> -pc_alloc_uninit(v9p->alloc);
> +qvirtqueue_cleanup(_pci, v9p->vq, v9p->qs->alloc);
>  qvirtio_pci_device_disable(container_of(v9p->dev, QVirtioPCIDevice, 
> vdev));
>  g_free(v9p->dev);
> -qpci_free_pc(v9p->bus);
>  g_free(v9p);
>  }
>  
> @@ -96,9 +89,11 @@ static void pci_basic_config(void)
>  size_t tag_len;
>  char *tag;
>  int i;
> +QOSState *qs;
>  
> -qvirtio_9p_start();
> -v9p = qvirtio_9p_pci_init();
> +qs = qvirtio_9p_start();
> +g_assert(qs);
> +v9p = qvirtio_9p_pci_init(qs);
>  
>  addr = ((QVirtioPCIDevice *) v9p->dev)->addr + 
> VIRTIO_PCI_CONFIG_OFF(false);
>  tag_len = qvirtio_config_readw(_pci, v9p->dev,
> @@ -115,7 +110,7 @@ static void pci_basic_config(void)
>  g_free(tag);
>  
>  qvirtio_9p_pci_free(v9p);
> -qvirtio_9p_stop();
> +qvirtio_9p_stop(qs);
>  }
>  
>  int main(int argc, char **argv)
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index 3c4fecc..8cf62f6 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -10,12 +10,10 @@
>  
>  #include "qemu/osdep.h"
>  #include "libqtest.h"
> +#include "libqos/libqos-pc.h"
>  #include "libqos/virtio.h"
>  #include "libqos/virtio-pci.h"
>  #include "libqos/virtio-mmio.h"
> -#include "libqos/pci-pc.h"
> -#include "libqos/malloc.h"
> -#include "libqos/malloc-pc.h"
>  #include "libqos/malloc-generic.h"
>  #include "qemu/bswap.h"
>  #include 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/3] qtest: evaluate endianness of the target in qtest_init()

[David Gibson]

On Thu, Sep 29, 2016 at 07:15:06PM +0200, Laurent Vivier wrote:
> This allows to store it and not have to rescan the list
> each time we need it.
> 
> Signed-off-by: Laurent Vivier 
> Reviewed-by: Greg Kurz 

Reviewed-by: David Gibson 

In that it looks technically correct.  The whole notion of guest
endianness is kind of bogus, doubly so when we're essentially
replacing the cpu with the qtest proxy.  It'd be better to make all
the io operations have explicit endianness.

Still, it looks reasonable in the short term.

> ---
>  tests/libqos/virtio-pci.c |  2 +-
>  tests/libqtest.c  | 96 
> +--
>  tests/libqtest.h  | 16 ++--
>  tests/virtio-blk-test.c   |  2 +-
>  4 files changed, 66 insertions(+), 50 deletions(-)
> 
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index 18b92b9..6e005c1 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -86,7 +86,7 @@ static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, 
> uint64_t addr)
>  int i;
>  uint64_t u64 = 0;
>  
> -if (qtest_big_endian()) {
> +if (target_big_endian()) {
>  for (i = 0; i < 8; ++i) {
>  u64 |= (uint64_t)qpci_io_readb(dev->pdev,
>  (void *)(uintptr_t)addr + i) << (7 - i) * 8;
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 6f6bdf1..aa4bc9e 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -37,6 +37,7 @@ struct QTestState
>  bool irq_level[MAX_IRQ];
>  GString *rx;
>  pid_t qemu_pid;  /* our child QEMU process */
> +bool big_endian;
>  };
>  
>  static GHookList abrt_hooks;
> @@ -146,6 +147,52 @@ void qtest_add_abrt_handler(GHookFunc fn, const void 
> *data)
>  g_hook_prepend(_hooks, hook);
>  }
>  
> +static bool arch_is_big_endian(const char *arch)
> +{
> +int i;
> +static const struct {
> +const char *arch;
> +bool big_endian;
> +} endianness[] = {
> +{ "aarch64", false },
> +{ "alpha", false },
> +{ "arm", false },
> +{ "cris", false },
> +{ "i386", false },
> +{ "lm32", true },
> +{ "m68k", true },
> +{ "microblaze", true },
> +{ "microblazeel", false },
> +{ "mips", true },
> +{ "mips64", true },
> +{ "mips64el", false },
> +{ "mipsel", false },
> +{ "moxie", true },
> +{ "or32", true },
> +{ "ppc", true },
> +{ "ppc64", true },
> +{ "ppcemb", true },
> +{ "s390x", true },
> +{ "sh4", false },
> +{ "sh4eb", true },
> +{ "sparc", true },
> +{ "sparc64", true },
> +{ "unicore32", false },
> +{ "x86_64", false },
> +{ "xtensa", false },
> +{ "xtensaeb", true },
> +{ "tricore", false },
> +{},
> +};
> +
> +for (i = 0; endianness[i].arch; i++) {
> +if (strcmp(endianness[i].arch, arch) == 0) {
> +return endianness[i].big_endian;
> +}
> +}
> +g_assert_not_reached();
> +}
> +
>  QTestState *qtest_init(const char *extra_args)
>  {
>  QTestState *s;
> @@ -209,6 +256,8 @@ QTestState *qtest_init(const char *extra_args)
>  kill(s->qemu_pid, SIGSTOP);
>  }
>  
> +s->big_endian = arch_is_big_endian(qtest_get_arch());
> +
>  return s;
>  }
>  
> @@ -886,50 +935,7 @@ char *hmp(const char *fmt, ...)
>  return ret;
>  }
>  
> -bool qtest_big_endian(void)
> +bool qtest_big_endian(QTestState *s)
>  {
> -const char *arch = qtest_get_arch();
> -int i;
> -
> -static const struct {
> -const char *arch;
> -bool big_endian;
> -} endianness[] = {
> -{ "aarch64", false },
> -{ "alpha", false },
> -{ "arm", false },
> -{ "cris", false },
> -{ "i386", false },
> -{ "lm32", true },
> -{ "m68k", true },
> -{ "microblaze", true },
> -{ "microblazeel", false },
> -{ "mips", true },
> -{ "mips64", true },
> -{ "mips64el", false },
> -{ "mipsel", false },
> -{ "moxie", true },
> -{ "or32", true },
> -{ "ppc", true },
> -{ "ppc64", true },
> -{ "ppcemb", true },
> -{ "s390x", true },
> -{ "sh4", false },
> -{ "sh4eb", true },
> -{ "sparc", true },
> -{ "sparc64", true },
> -{ "unicore32", false },
> -{ "x86_64", false },
> -{ "xtensa", false },
> -{ "xtensaeb", true },
> -{},
> -};
> -
> -for (i = 0; endianness[i].arch; i++) {
> -if (strcmp(endianness[i].arch, arch) == 0) {
> -return endianness[i].big_endian;
> -}
> -}
> -
> -return false;
> +return s->big_endian;
>  }
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index f7402e0..4be1f77 100644
> --- 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/3] tests: enable virtio tests on SPAPR

[David Gibson]

On Thu, Sep 29, 2016 at 07:15:07PM +0200, Laurent Vivier wrote:
> but disable MSI-X tests on SPAPR as we can't check the result
> (the memory region used on PC is not readable on SPAPR).
> 
> Signed-off-by: Laurent Vivier 
> ---
>  tests/Makefile.include|  3 ++-
>  tests/libqos/virtio-pci.c | 22 --
>  tests/virtio-9p-test.c| 11 ++-
>  tests/virtio-blk-test.c   | 22 +-
>  tests/virtio-net-test.c   | 17 +++--
>  tests/virtio-rng-test.c   |  7 ++-
>  tests/virtio-scsi-test.c  | 10 +-
>  7 files changed, 79 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index c46a32d..1e4a3d5 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -278,6 +278,7 @@ check-qtest-ppc64-y += tests/usb-hcd-uhci-test$(EXESUF)
>  gcov-files-ppc64-y += hw/usb/hcd-uhci.c
>  check-qtest-ppc64-y += tests/usb-hcd-xhci-test$(EXESUF)
>  gcov-files-ppc64-y += hw/usb/hcd-xhci.c
> +check-qtest-ppc64-y += $(check-qtest-virtio-y)
>  
>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>  
> @@ -604,7 +605,7 @@ libqos-pc-obj-y += tests/libqos/ahci.o
>  libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o
>  libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o
>  libqos-usb-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) 
> tests/libqos/usb.o
> -libqos-virtio-obj-y = $(libqos-pc-obj-y) tests/libqos/virtio.o 
> tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o 
> tests/libqos/malloc-generic.o
> +libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) 
> tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o 
> tests/libqos/malloc-generic.o
>  
>  tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
>  tests/rtc-test$(EXESUF): tests/rtc-test.o
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index 6e005c1..ed81df6 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -68,16 +68,34 @@ static uint8_t qvirtio_pci_config_readb(QVirtioDevice *d, 
> uint64_t addr)
>  return qpci_io_readb(dev->pdev, (void *)(uintptr_t)addr);
>  }
>  
> +/* PCI is always read in little-endian order
> + * but virtio ( < 1.0) is in guest order
> + * so with a big-endian guest the order has been reversed
> + * reverse it again
> + */
> +
>  static uint16_t qvirtio_pci_config_readw(QVirtioDevice *d, uint64_t addr)
>  {
>  QVirtioPCIDevice *dev = (QVirtioPCIDevice *)d;
> -return qpci_io_readw(dev->pdev, (void *)(uintptr_t)addr);
> +uint16_t value;
> +
> +value = qpci_io_readw(dev->pdev, (void *)(uintptr_t)addr);
> +if (target_big_endian()) {
> +value = bswap16(value);
> +}

Don't you need some sort of test to distinguish the virtio < 1.0 and
virtio-1.0 cases?

> +return value;
>  }
>  
>  static uint32_t qvirtio_pci_config_readl(QVirtioDevice *d, uint64_t addr)
>  {
>  QVirtioPCIDevice *dev = (QVirtioPCIDevice *)d;
> -return qpci_io_readl(dev->pdev, (void *)(uintptr_t)addr);
> +uint32_t value;
> +
> +value = qpci_io_readl(dev->pdev, (void *)(uintptr_t)addr);
> +if (target_big_endian()) {
> +value = bswap32(value);
> +}
> +return value;
>  }
>  
>  static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, uint64_t addr)
> diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
> index 28d7f5b..a73bccb 100644
> --- a/tests/virtio-9p-test.c
> +++ b/tests/virtio-9p-test.c
> @@ -11,6 +11,7 @@
>  #include "libqtest.h"
>  #include "qemu-common.h"
>  #include "libqos/libqos-pc.h"
> +#include "libqos/libqos-spapr.h"
>  #include "libqos/virtio.h"
>  #include "libqos/virtio-pci.h"
>  #include "standard-headers/linux/virtio_ids.h"
> @@ -22,12 +23,20 @@ static char *test_share;
>  
>  static QOSState *qvirtio_9p_start(void)
>  {
> +const char *arch = qtest_get_arch();
> +QOSState *qs = NULL;
>  test_share = g_strdup("/tmp/qtest.XX");
>  g_assert_nonnull(mkdtemp(test_share));
>  const char *cmd = "-fsdev local,id=fsdev0,security_model=none,path=%s "
>"-device virtio-9p-pci,fsdev=fsdev0,mount_tag=%s";
>  
> -return qtest_pc_boot(cmd, test_share, mount_tag);
> +if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +qs = qtest_pc_boot(cmd, test_share, mount_tag);
> +} else if (strcmp(arch, "ppc64") == 0) {
> +qs = qtest_spapr_boot(cmd, test_share, mount_tag);
> +}
> +
> +return qs;
>  }
>  
>  static void qvirtio_9p_stop(QOSState *qs)
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index a4de3e4..a032f8e 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -11,6 +11,7 @@
>  #include "qemu/osdep.h"
>  #include "libqtest.h"
>  #include "libqos/libqos-pc.h"
> +#include "libqos/libqos-spapr.h"
>  #include "libqos/virtio.h"
>  #include "libqos/virtio-pci.h"
>  #include "libqos/virtio-mmio.h"
> @@ -58,6 +59,7 

Re: [Qemu-devel] [PATCH 3/3] target-arm: Comments to mark location of pending work for 56 bit addresses

[Peter Maydell]

On 16 September 2016 at 10:34, Thomas Hanson  wrote:
> Certain instructions which can not directly load a tagged address value
> may trigger a corner case when the address size is 56 bits.  This is
> because incrementing or offsetting from the current PC can cause an
> arithetic roll-over into the tag bits.  Per the ARM ARM spec, these cases
> should also be addressed by cleaning up the tag field.
>
> This work was not done at this time since the changes could not be tested
> with current CPU models.  Comments have been added to flag the locations
> where this will need to be fixed once a model is available.

This is *not* why we haven't done this work. We haven't done it
because the maximum virtual address size permitted by the
architecture is less than 56 bits, and so this is a "can't happen"
situation.

> 3 comments added in same file to identify cases in a switch.

This should be a separate patch, because it is unrelated to the
tagged address stuff.

> Signed-off-by: Thomas Hanson 
> ---
>  target-arm/translate-a64.c | 18 +++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 4d6f951..8810180 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1205,6 +1205,9 @@ static inline AArch64DecodeFn *lookup_disas_fn(const 
> AArch64DecodeTable *table,
>   */
>  static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
>  {
> +/*If/when address size is 56 bits, this could overflow into address tag

Missing space before "If".

> + * byte, and that byte should be fixed per ARM ARM spec.
> + */
>  uint64_t addr = s->pc + sextract32(insn, 0, 26) * 4 - 4;
>
>  if (insn & (1U << 31)) {
> @@ -1232,6 +1235,9 @@ static void disas_comp_b_imm(DisasContext *s, uint32_t 
> insn)
>  sf = extract32(insn, 31, 1);
>  op = extract32(insn, 24, 1); /* 0: CBZ; 1: CBNZ */
>  rt = extract32(insn, 0, 5);
> +/*If/when address size is 56 bits, this could overflow into address tag
> + * byte, and that byte should be fixed per ARM ARM spec.
> + */
>  addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
>
>  tcg_cmp = read_cpu_reg(s, rt, sf);
> @@ -1260,6 +1266,9 @@ static void disas_test_b_imm(DisasContext *s, uint32_t 
> insn)
>
>  bit_pos = (extract32(insn, 31, 1) << 5) | extract32(insn, 19, 5);
>  op = extract32(insn, 24, 1); /* 0: TBZ; 1: TBNZ */
> +/*If/when address size is 56 bits, this could overflow into address tag
> + * byte, and that byte should be fixed per ARM ARM spec.
> + */
>  addr = s->pc + sextract32(insn, 5, 14) * 4 - 4;
>  rt = extract32(insn, 0, 5);
>
> @@ -1289,6 +1298,9 @@ static void disas_cond_b_imm(DisasContext *s, uint32_t 
> insn)
>  unallocated_encoding(s);
>  return;
>  }
> +/*If/when address size is 56 bits, this could overflow into address tag
> + * byte, and that byte should be fixed per ARM ARM spec.
> + */
>  addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
>  cond = extract32(insn, 0, 4);
>
> @@ -1636,12 +1648,12 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>   * instruction works properly.
>   */
>  switch (op2_ll) {
> -case 1:
> +case 1: /* SVC */
>  gen_ss_advance(s);
>  gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16),
> default_exception_el(s));
>  break;
> -case 2:
> +case 2: /* HVC */
>  if (s->current_el == 0) {
>  unallocated_encoding(s);
>  break;
> @@ -1654,7 +1666,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>  gen_ss_advance(s);
>  gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16), 2);
>  break;
> -case 3:
> +case 3: /* SMC */
>  if (s->current_el == 0) {
>  unallocated_encoding(s);
>  break;
> --
> 1.9.1

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] target-arm: Code changes to implement overwrite of tag field on PC load

[Peter Maydell]

On 16 September 2016 at 10:34, Thomas Hanson  wrote:
> gen_intermediate_code_a64() transfers TBI values from TB->flags to
> DisasContext structure.
>
> disas_uncond_b_reg() calls new function gen_a64_set_pc_reg() to handle BR,
> BLR and RET instructions.
>
> gen_a64_set_pc_reg() implements all of the required checks and overwiting
> logic to clean up the tag field of an address before loading the PC.
> Currently only called in one place, but will be used in the future to
> handle arithmetic overflow cases with 56-bit addresses.  (See following
> patch.)

I've cc'd RTH in case he wants to comment on the TCG op sequences
we're generating here.

Same commit message style remarks as patch 1.

> Signed-off-by: Thomas Hanson 
> ---
>  target-arm/translate-a64.c | 67 
> ++
>  1 file changed, 62 insertions(+), 5 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index f5e29d2..4d6f951 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -41,6 +41,7 @@ static TCGv_i64 cpu_pc;
>
>  /* Load/store exclusive handling */
>  static TCGv_i64 cpu_exclusive_high;
> +static TCGv_i64 cpu_reg(DisasContext *s, int reg);
>
>  static const char *regnames[] = {
>  "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7",
> @@ -176,6 +177,58 @@ void gen_a64_set_pc_im(uint64_t val)
>  tcg_gen_movi_i64(cpu_pc, val);
>  }
>
> +void gen_a64_set_pc_reg(DisasContext *s, unsigned int rn)

I think it would be better to take a TCGv_i64 here rather than
unsigned int rn (ie have the caller do the cpu_reg(s, rn)).
(You probably don't need that prototype of cpu_reg() above if
you do this, though that's not why it's better.)

> +{

This could use a brief comment explaining the semantics we are
implementing here, something like:

/* Load the PC from a register.
 *
 * If address tagging is enabled via the TCR TBI bits, then loading
 * an address into the PC will clear out any tag in the it:
 *  + for EL2 and EL3 there is only one TBI bit, and if it is set
 *then the address is zero-extended, clearing bits [63:56]
 *  + for EL0 and EL1, TBI0 controls addresses with bit 55 == 0
 *and TBI1 controls addressses with bit 55 == 1.
 *If the appropriate TBI bit is set for the address then
 *the address is sign-extended from bit 55 into bits [63:56]
 *
 * We can avoid doing this for relative-branches, because the
 * PC + offset can never overflow into the tag bits (assuming
 * that virtual addresses are less than 56 bits wide, as they
 * are currently), but we must handle it for branch-to-register.
 */

> +if (s->current_el <= 1) {
> +/* Test if NEITHER or BOTH TBI values are set.  If so, no need to
> + * examine bit 55 of address, can just generate code.
> + * If mixed, then test via generated code
> + */
> +if (s->tbi0 && s->tbi1) {
> +TCGv_i64 tmp_reg = tcg_temp_new_i64();
> +/* Both bits set, just fix it */

Rather than "just fix it", we should say "always sign extend from
bit 55 into [63:56]".

> +tcg_gen_shli_i64(tmp_reg, cpu_reg(s, rn), 8);
> +tcg_gen_sari_i64(cpu_pc, tmp_reg, 8);
> +tcg_temp_free_i64(tmp_reg);
> +} else if (!s->tbi0 && !s->tbi1) {
> +/* Neither bit set, just load it as-is */
> +tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn));
> +} else {
> +TCGv_i64 tcg_tmpval = tcg_temp_new_i64();
> +TCGv_i64 tcg_bit55  = tcg_temp_new_i64();
> +TCGv_i64 tcg_zero   = tcg_const_i64(0);
> +
> +tcg_gen_andi_i64(tcg_bit55, cpu_reg(s, rn), (1ull << 55));
> +
> +if (s->tbi0) {
> +/* tbi0==1, tbi1==0, so 0-fill upper byte if bit 55 = 0 */
> +tcg_gen_andi_i64(tcg_tmpval, cpu_reg(s, rn),
> + 0x00FFull);
> +tcg_gen_movcond_i64(TCG_COND_EQ, cpu_pc, tcg_bit55, tcg_zero,
> +tcg_tmpval, cpu_reg(s, rn));
> +} else {
> +/* tbi0==0, tbi1==1, so 1-fill upper byte if bit 55 = 1 */
> +tcg_gen_ori_i64(tcg_tmpval, cpu_reg(s, rn),
> +0xFF00ull);
> +tcg_gen_movcond_i64(TCG_COND_NE, cpu_pc, tcg_bit55, tcg_zero,
> +tcg_tmpval, cpu_reg(s, rn));
> +}
> +tcg_temp_free_i64(tcg_zero);
> +tcg_temp_free_i64(tcg_bit55);
> +tcg_temp_free_i64(tcg_tmpval);
> +}
> +} else {  /* EL > 1 */
> +if (s->tbi0) {
> +/* Force tag byte to all zero */
> +tcg_gen_andi_i64(cpu_pc, cpu_reg(s, rn), 0x00FFull);
> +} else {
> +/* Load unmodified address */
> +

Re: [Qemu-devel] [PATCH 1/3] target-arm: Infrastucture changes to enable handling of tagged address loading into PC

[Peter Maydell]

On 16 September 2016 at 10:34, Thomas Hanson  wrote:

This patch is mostly good; minor comments below.

> New arm_regime_tbi0() and arm_regime_tbi0() to extract the TBI values from
> the correct TCR for the current EL.
>
> New shift, mask and accessor macro definitions needed to add TBI flag bits
> to the TB flags field.
>
> cpu_get_tb_cpu_state() inserst the TBI values into 'flags' parameter
> so that they show up in the TB flags field.
>
> tbi0, tbi1 fields added to definition of DisasContext structure.

This is just describing the "what" of the change (which you
can find fairly easily by just looking at the patch). Commit
messages should concentrate on the "why" (and the 'body' part
should read OK standalone without having to read the 'subject
line' part too).

Put another way, this reads like a GCC changelog message :-)

> Signed-off-by: Thomas Hanson 
> ---
>  target-arm/cpu.h   | 20 ++--
>  target-arm/helper.c| 42 ++
>  target-arm/translate.h |  3 +++
>  3 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 76d824d..c2d6e75 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -2191,7 +2191,11 @@ static inline bool 
> arm_cpu_data_is_big_endian(CPUARMState *env)
>  #define ARM_TBFLAG_BE_DATA_SHIFT20
>  #define ARM_TBFLAG_BE_DATA_MASK (1 << ARM_TBFLAG_BE_DATA_SHIFT)
>
> -/* Bit usage when in AArch64 state: currently we have no A64 specific bits */
> +/* Bit usage when in AArch64 state */
> +#define ARM_TBFLAG_TBI0_SHIFT 0/* TBI0 for EL0/1 or TBI for EL2/3 */
> +#define ARM_TBFLAG_TBI0_MASK (0x1ull << ARM_TBFLAG_TBI0_SHIFT)
> +#define ARM_TBFLAG_TBI1_SHIFT 1/* TBI1 for EL0/1  */
> +#define ARM_TBFLAG_TBI1_MASK (0x1ull << ARM_TBFLAG_TBI1_SHIFT)
>
>  /* some convenience accessor macros */
>  #define ARM_TBFLAG_AARCH64_STATE(F) \
> @@ -,6 +2226,10 @@ static inline bool 
> arm_cpu_data_is_big_endian(CPUARMState *env)
>  (((F) & ARM_TBFLAG_NS_MASK) >> ARM_TBFLAG_NS_SHIFT)
>  #define ARM_TBFLAG_BE_DATA(F) \
>  (((F) & ARM_TBFLAG_BE_DATA_MASK) >> ARM_TBFLAG_BE_DATA_SHIFT)
> +#define ARM_TBFLAG_TBI0(F) \
> +(((F) & ARM_TBFLAG_TBI0_MASK) >> ARM_TBFLAG_TBI0_SHIFT)
> +#define ARM_TBFLAG_TBI1(F) \
> +(((F) & ARM_TBFLAG_TBI1_MASK) >> ARM_TBFLAG_TBI1_SHIFT)
>
>  static inline bool bswap_code(bool sctlr_b)
>  {
> @@ -2319,12 +2327,19 @@ static inline bool arm_cpu_bswap_data(CPUARMState 
> *env)
>  }
>  #endif
>
> +long arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx);
> +long arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx);

Why "long" (very rarely the right type in CPU emulation code
because its width depends on the host CPU architecture) ?

New global-scope function prototypes should have a standard-form
doc comment (we have a lot without, but I'm trying to improve
by the ratchet mechanism of insisting on prototypes for newly
added or modified ones).

>  static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>  target_ulong *cs_base, uint32_t 
> *flags)
>  {
> +ARMMMUIdx mmu_idx = cpu_mmu_index(env, false);
>  if (is_a64(env)) {
>  *pc = env->pc;
>  *flags = ARM_TBFLAG_AARCH64_STATE_MASK;
> +/* Get control bits for tagged addresses */
> +*flags |= (arm_regime_tbi0(env, mmu_idx) << ARM_TBFLAG_TBI0_SHIFT);
> +*flags |= (arm_regime_tbi1(env, mmu_idx) << ARM_TBFLAG_TBI1_SHIFT);
>  } else {
>  *pc = env->regs[15];
>  *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT)
> @@ -2343,7 +2358,8 @@ static inline void cpu_get_tb_cpu_state(CPUARMState 
> *env, target_ulong *pc,
> << ARM_TBFLAG_XSCALE_CPAR_SHIFT);
>  }
>
> -*flags |= (cpu_mmu_index(env, false) << ARM_TBFLAG_MMUIDX_SHIFT);
> +*flags |= (mmu_idx << ARM_TBFLAG_MMUIDX_SHIFT);
> +
>  /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine
>   * states defined in the ARM ARM for software singlestep:
>   *  SS_ACTIVE   PSTATE.SS   State
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index bdb842c..526306e 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6720,6 +6720,48 @@ static inline TCR *regime_tcr(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>  return >cp15.tcr_el[regime_el(env, mmu_idx)];
>  }
>
> +/* Returns TBI0 value for current regime el */
> +long arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +TCR*tcr;

Weird spacing.

> +uint32_t el;
> +
> +/* regime_el fails for mmu_idx = ARMMMUIdx_S12NSE0 or ARMMMUIdx_S12NSE1 
> */

Better comment:
   /* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
* a stage 1+2 mmu index into the appropriate stage 1 mmu index.
*/

> +if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> +mmu_idx += ARMMMUIdx_S1NSE0;
> 

Re: [Qemu-devel] [Qemu-ppc] How to add my implementation of the fmadds instruction to QEMU

[Programmingkid]

On Sep 29, 2016, at 8:39 PM, David Gibson wrote:

> On Thu, Sep 29, 2016 at 12:55:23PM -0400, Programmingkid wrote:
>> 
>> On Sep 29, 2016, at 11:41 AM, Peter Maydell wrote:
>> 
>>> On 28 September 2016 at 21:17, David Gibson  
>>> wrote:
 I think there is a way you could get both speed and accuracy, but it's
 a huge project:
 
 You'd need to add full float awareness to TCG - so floating point TCG
 values and floating point operations as tcp micro-ops, defined
 according to IEEE semantics.  Then you'd need to rewrite the TCG
 frontends in terms of those new ops, at least for target CPUs close
 enough to IEEE semantics for that to work.  And you'd need to rewrite
 the TCG backends to implement those fp ops in terms of host cpu fp
 instructions .. at least when the host has fp behaviour close enough
 to IEEE to make that work, with a fallback to soft float when that's
 not the case.
>>> 
>>> Also even if you have float support in both frontend and backend
>>> you still need to fall back to fully-emulated for the runtime
>>> corner cases (like where tininess before/after rounding makes a
>>> difference or where you need to care about minutiae of the
>>> floating point exception flags, etc). It's not impossible
>>> but it is a very large amount of technically complicated work.
>> 
>> 
>> This project sounds like it should have its own web page. Maybe even
>> its own Google Summer of Code entry. I created a mindmap of
>> this project. The picture is attached to this email. This is
>> just a start. Please let me know what should be added or changed.
> 
> TBH, I think this is rather bigger than a GSoC project.

If it is really big, then it should be broken down into easier steps.
Any idea what those steps could be?

Re: [Qemu-devel] [PATCH] target-i386: Report known CPUID[EAX=0xD, ECX=0]:EAX bits as migratable

[Wanpeng Li]

2016-09-30 2:40 GMT+08:00 Eduardo Habkost :
> (CCing Richard, sorry I forgot to CC you)
>
> Ping? Any objection to this fix?
>
> On Wed, Sep 28, 2016 at 01:33:15PM -0300, Eduardo Habkost wrote:
>> A regression was introduced by commit 96193c22a "target-i386:
>> Move xsave component mask to features array": all
>> CPUID[EAX=0xD,ECX=0]:EAX bits were being reported as unmigratable
>> because they don't have feature names defined. This broke
>> "-cpu host" because it enables only migratable features by
>> default.
>>
>> This adds a new field to FeatureWordInfo: migratable_flags, which
>> will make those features be reported as migratable even if they
>> don't have a property name defined.
>>
>> Reported-by: Wanpeng Li 
>> Cc: Paolo Bonzini 
>> Signed-off-by: Eduardo Habkost 

Reviewed-by: Wanpeng Li 

>> ---
>>  target-i386/cpu.c | 19 +++
>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 09b..0807e92 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -258,6 +258,7 @@ typedef struct FeatureWordInfo {
>>  int cpuid_reg;/* output register (R_* constant) */
>>  uint32_t tcg_features; /* Feature flags supported by TCG */
>>  uint32_t unmigratable_flags; /* Feature flags known to be unmigratable 
>> */
>> +uint32_t migratable_flags; /* Feature flags known to be migratable */
>>  } FeatureWordInfo;
>>
>>  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>> @@ -494,6 +495,10 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] 
>> = {
>>  .cpuid_needs_ecx = true, .cpuid_ecx = 0,
>>  .cpuid_reg = R_EAX,
>>  .tcg_features = ~0U,
>> +.migratable_flags = XSTATE_FP_MASK | XSTATE_SSE_MASK |
>> +XSTATE_YMM_MASK | XSTATE_BNDREGS_MASK | XSTATE_BNDCSR_MASK |
>> +XSTATE_OPMASK_MASK | XSTATE_ZMM_Hi256_MASK | 
>> XSTATE_Hi16_ZMM_MASK |
>> +XSTATE_PKRU_MASK,
>>  },
>>  [FEAT_XSAVE_COMP_HI] = {
>>  .cpuid_eax = 0xD,
>> @@ -600,15 +605,13 @@ static uint32_t 
>> x86_cpu_get_migratable_flags(FeatureWord w)
>>
>>  for (i = 0; i < 32; i++) {
>>  uint32_t f = 1U << i;
>> -/* If the feature name is unknown, it is not supported by QEMU yet 
>> */
>> -if (!wi->feat_names[i]) {
>> -continue;
>> -}
>> -/* Skip features known to QEMU, but explicitly marked as 
>> unmigratable */
>> -if (wi->unmigratable_flags & f) {
>> -continue;
>> +
>> +/* If the feature name is known, it is implicitly considered 
>> migratable,
>> + * unless it is explicitly set in unmigratable_flags */
>> +if ((wi->migratable_flags & f) ||
>> +(wi->feat_names[i] && !(wi->unmigratable_flags & f))) {
>> +r |= f;
>>  }
>> -r |= f;
>>  }
>>  return r;
>>  }
>> --
>> 2.7.4
>>
>>
>
> --
> Eduardo

Re: [Qemu-devel] [PATCH v8 6/8] STM32F205: Connect the ADC devices

[Alistair Francis]

On Thu, Sep 29, 2016 at 4:59 PM, Peter Maydell  wrote:
> On 24 September 2016 at 12:20, Alistair Francis  wrote:
>> Connect the ADC devices to the STM32F205 SoC.
>>
>> Signed-off-by: Alistair Francis 
>> ---
>>
>>  #define FLASH_BASE_ADDRESS 0x0800
>>  #define FLASH_SIZE (1024 * 1024)
>> @@ -52,6 +55,9 @@ typedef struct STM32F205State {
>>  STM32F2XXSyscfgState syscfg;
>>  STM32F2XXUsartState usart[STM_NUM_USARTS];
>>  STM32F2XXTimerState timer[STM_NUM_TIMERS];
>> +STM32F2XXADCState adc[STM_NUM_ADCS];
>> +
>> +qemu_or_irq *adc_irqs;
>
> Seems mildly inconsistent that all the other devices
> "owned" by this SoC are embedded in the struct but this
> one is a pointer (and so initialized via object_new()
> rather than object_initialized()).
>
> That's not a big deal though, so I've applied this
> series to target-arm.next; you can change the above
> in a followup patch, or not, as you choose.

Good point, I didn't really think of it like that.

I don't mind it this way, hopefully I will set up another patch series
and I'll update it then if need be.

Thanks,

Alistair

>
>>  } STM32F205State;
>>
>>  #endif
>> --
>> 2.7.4
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v7 5/8] hw/intc/arm_gicv3_its: Implement support for in-kernel ITS emulation

[Peter Maydell]

On 29 September 2016 at 17:38, Peter Maydell  wrote:
> On 23 September 2016 at 00:43, Eric Auger  wrote:
>> From: Pavel Fedin 
>>
>> The ITS control frame is in-kernel emulated while accesses to the
>> GITS_TRANSLATER are mediated through the KVM_SIGNAL_MSI ioctl (MSI
>> direct MSI injection advertised by the CAP_SIGNAL_MSI capability)
>>
>> the kvm_gsi_direct_mapping is explicitly set to false to emphasize the
>> difference with GICv2M. Direct mapping cannot work with ITS since
>> the content of the MSI data is not the target interrupt ID but an
>> eventd id.
>>
>> GSI routing is advertised (kvm_gsi_routing_allowed) as well as
>> msi/irqfd signaling (kvm_msi_via_irqfd_allowed).
>>
>> The MSI frame (GITS_TRANSLATER) absolute GPA is computed on first
>> kvm_its_send_msi() call. It is then passed through KVM_SIGNAL_MSI
>> ioctl.
>>
>> Signed-off-by: Pavel Fedin 
>> Signed-off-by: Eric Auger 
>
>
>
> Applied to target-arm.next, thanks.

Oops, meant to send that as a followup to the cover letter!

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 5/8] irq: Add a new irq device that allows the ORing of lines

[Alistair Francis]

On Thu, Sep 29, 2016 at 4:54 PM, Peter Maydell  wrote:
> On 24 September 2016 at 12:20, Alistair Francis  wrote:
>> Signed-off-by: Alistair Francis 
>> ---
>> As the migration framework is not included in user mode this needs to be a
>> new file.
>>
>> V8:
>>  - Use the standard qdev_init_gpio_in() function
>> V7:
>>  - Use the standard QEMU init/realise functions
>> V6:
>>  - Make the OR IRQ device a TYPE_DEVICE
>>  - Add vmstate
>
> Reviewed-by: Peter Maydell 

Thanks!

>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v7 5/8] hw/intc/arm_gicv3_its: Implement support for in-kernel ITS emulation

[Peter Maydell]

On 23 September 2016 at 00:43, Eric Auger  wrote:
> From: Pavel Fedin 
>
> The ITS control frame is in-kernel emulated while accesses to the
> GITS_TRANSLATER are mediated through the KVM_SIGNAL_MSI ioctl (MSI
> direct MSI injection advertised by the CAP_SIGNAL_MSI capability)
>
> the kvm_gsi_direct_mapping is explicitly set to false to emphasize the
> difference with GICv2M. Direct mapping cannot work with ITS since
> the content of the MSI data is not the target interrupt ID but an
> eventd id.
>
> GSI routing is advertised (kvm_gsi_routing_allowed) as well as
> msi/irqfd signaling (kvm_msi_via_irqfd_allowed).
>
> The MSI frame (GITS_TRANSLATER) absolute GPA is computed on first
> kvm_its_send_msi() call. It is then passed through KVM_SIGNAL_MSI
> ioctl.
>
> Signed-off-by: Pavel Fedin 
> Signed-off-by: Eric Auger 



Applied to target-arm.next, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v1 1/1] cadence_gem: Fix priority queue out of bounds access

[Alistair Francis]

On Thu, Sep 29, 2016 at 5:04 PM, Peter Maydell  wrote:
> On 26 September 2016 at 10:49, Alistair Francis
>  wrote:
>> There was an error with some of the register implementation assuming
>> there are 16 priority queues supported when the IP only supports 8. This
>> patch corrects the registers to only support 8 queues.
>>
>> Signed-off-by: Alistair Francis 
>> Reported-by: Paolo Bonzini 
>> ---
>> Thanks to Paolo for pointing this out.
>>
>>  hw/net/cadence_gem.c | 22 --
>>  1 file changed, 4 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 8618e7a..7915732 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -147,25 +147,19 @@
>>  #define GEM_INT_Q1_MASK (0x0640 / 4)
>>
>>  #define GEM_TRANSMIT_Q1_PTR (0x0440 / 4)
>> -#define GEM_TRANSMIT_Q15_PTR(GEM_TRANSMIT_Q1_PTR + 14)
>> +#define GEM_TRANSMIT_Q7_PTR (GEM_TRANSMIT_Q1_PTR + 6)
>
> 8 queues but they're numbered one to ... seven ??

Yeah, I double checked the user guide and it is 1 to 7.

I guess queue 0 uses the standard registers.

Thanks,

Alistair

>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v7 0/8] vITS support

[Peter Maydell]

On 23 September 2016 at 00:43, Eric Auger  wrote:
> This series introduces support for in-kernel GICv3 ITS emulation.
>
> On dt guest the functionality is complete and was tested on Cavium ThunderX
> with virtio-net-pci and vhost-net.
>
> On ACPI guest the series was tested with virtio-net-pci only. For vhost-net,
> using MSIX we currently miss the ACPI IORT table generation linking the
> PCIe host controller with the ITS. The work is ongoing and will be submitted
> separately. Anyway the kernel ACPI IORT ITS node support is not upstreamed
> yet.
>
> The first patch is not really related to virtual ITS but advertises the KVM
> GSI routing support which goes along with MSI injection.
>
> For ACPI ITS and PCIe support, use in-flight Tomasz' series:
> - [PATCH V10 0/8] Introduce ACPI world to ITS,
>   https://lkml.org/lkml/2016/9/6/153
> - Support for ARM64 ACPI based PCI host controller,
>   https://lwn.net/Articles/690995/
>
> Git Information:
> v7: https://github.com/eauger/qemu/tree/v2.7.0-vITS-v7
> v6: https://github.com/eauger/qemu/tree/v2.7.0-vITS-v6

Applied to target-arm.next, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v7 4/8] kvm-all: Pass requester ID to MSI routing functions

[Peter Maydell]

On 23 September 2016 at 00:43, Eric Auger  wrote:
> From: Pavel Fedin 
>
> Introduce global kvm_msi_use_devid flag plus associated
> kvm_msi_devid_required() macro. Passes the device ID,
> if needed, while building the MSI route entry. Device IDs are
> required by the ARM GICv3 ITS (IRQ remapping function is based on
> this information).
>
> Signed-off-by: Pavel Fedin 
> Signed-off-by: Eric Auger 
>

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [Qemu-ppc] How to add my implementation of the fmadds instruction to QEMU

[David Gibson]

On Thu, Sep 29, 2016 at 12:55:23PM -0400, Programmingkid wrote:
> 
> On Sep 29, 2016, at 11:41 AM, Peter Maydell wrote:
> 
> > On 28 September 2016 at 21:17, David Gibson  
> > wrote:
> >> I think there is a way you could get both speed and accuracy, but it's
> >> a huge project:
> >> 
> >> You'd need to add full float awareness to TCG - so floating point TCG
> >> values and floating point operations as tcp micro-ops, defined
> >> according to IEEE semantics.  Then you'd need to rewrite the TCG
> >> frontends in terms of those new ops, at least for target CPUs close
> >> enough to IEEE semantics for that to work.  And you'd need to rewrite
> >> the TCG backends to implement those fp ops in terms of host cpu fp
> >> instructions .. at least when the host has fp behaviour close enough
> >> to IEEE to make that work, with a fallback to soft float when that's
> >> not the case.
> > 
> > Also even if you have float support in both frontend and backend
> > you still need to fall back to fully-emulated for the runtime
> > corner cases (like where tininess before/after rounding makes a
> > difference or where you need to care about minutiae of the
> > floating point exception flags, etc). It's not impossible
> > but it is a very large amount of technically complicated work.
> 
> 
> This project sounds like it should have its own web page. Maybe even
> its own Google Summer of Code entry. I created a mindmap of
> this project. The picture is attached to this email. This is
> just a start. Please let me know what should be added or changed.

TBH, I think this is rather bigger than a GSoC project.

-- 
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: [Qemu-devel] [PATCH v2] CODING_STYLE: Fix a typo ("have" vs. "has")

[Peter Maydell]

On 29 September 2016 at 17:04, Jonathan Neuschäfer
 wrote:
> Signed-off-by: Jonathan Neuschäfer 
>
> ---
> v2:
> - Preserve the poetic sound of "Many a flamewar", which Peter Maydell
>   pointed out.
> ---
>  CODING_STYLE | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index e7fde15..f53180b 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -9,7 +9,7 @@ patches before submitting.
>  Of course, the most important aspect in any coding style is whitespace.
>  Crusty old coders who have trouble spotting the glasses on their noses
>  can tell the difference between a tab and eight spaces from a distance
> -of approximately fifteen parsecs.  Many a flamewar have been fought and
> +of approximately fifteen parsecs.  Many a flamewar has been fought and
>  lost on this issue.
>
>  QEMU indents are four spaces.  Tabs are never used, except in Makefiles

Reviewed-by: Peter Maydell 

thanks
-- PMM

[Qemu-devel] [PATCH v13 2/2] docs: Add a generic loader explanation document

[Alistair Francis]

Signed-off-by: Alistair Francis 
Reviewed-by: Peter Maydell 
---
V11:
 - Fix corrections
V10:
 - Split the data loading and PC setting
V9:
 - Clarify the image loading options
V8:
 - Improve documentation
V6:
 - Fixup documentation
V4:
 - Re-write to be more comprehensive

 docs/generic-loader.txt | 84 +
 1 file changed, 84 insertions(+)
 create mode 100644 docs/generic-loader.txt

diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
new file mode 100644
index 000..8fcb550
--- /dev/null
+++ b/docs/generic-loader.txt
@@ -0,0 +1,84 @@
+Copyright (c) 2016 Xilinx Inc.
+
+This work is licensed under the terms of the GNU GPL, version 2 or later.  See
+the COPYING file in the top-level directory.
+
+
+The 'loader' device allows the user to load multiple images or values into
+QEMU at startup.
+
+Loading Data into Memory Values
+-
+The loader device allows memory values to be set from the command line. This
+can be done by following the syntax below:
+
+ -device loader,addr=,data=,data-len=
+   [,data-be=][,cpu-num=]
+
+  - The address to store the data in.
+  - The value to be written to the address. The maximum size of
+  the data is 8 bytes.
+  - The length of the data in bytes. This argument must be
+  included if the data argument is.
+   - Set to true if the data to be stored on the guest should be
+  written as big endian data. The default is to write little
+  endian data.
+   - The number of the CPU's address space where the data should
+  be loaded. If not specified the address space of the first
+  CPU is used.
+
+All values are parsed using the standard QemuOps parsing. This allows the user
+to specify any values in any format supported. By default the values
+will be parsed as decimal. To use hex values the user should prefix the number
+with a '0x'.
+
+An example of loading value 0x800e to address 0xfd1a0104 is:
+-device loader,addr=0xfd1a0104,data=0x800e,data-len=4
+
+Setting a CPU's Program Counter
+-
+The loader device allows the CPU's PC to be set from the command line. This
+can be done by following the syntax below:
+
+ -device loader,addr=,cpu-num=
+
+  - The value to use as the CPU's PC.
+   - The number of the CPU whose PC should be set to the
+  specified value.
+
+All values are parsed using the standard QemuOps parsing. This allows the user
+to specify any values in any format supported. By default the values
+will be parsed as decimal. To use hex values the user should prefix the number
+with a '0x'.
+
+An example of setting CPU 0's PC to 0x8000 is:
+-device loader,addr=0x8000,cpu-num=0
+
+Loading Files
+-
+The loader device also allows files to be loaded into memory. This can be done
+similarly to setting memory values. The syntax is shown below:
+
+-device 
loader,file=[,addr=][,cpu-num=][,force-raw=]
+
+  - A file to be loaded into memory
+  - The addr in memory that the file should be loaded. This is
+  ignored if you are using an ELF (unless force-raw is true).
+  This is required if you aren't loading an ELF.
+   - This specifies the CPU that should be used. This is an
+  optional argument and will cause the CPU's PC to be set to
+  where the image is stored or in the case of an ELF file to
+  the value in the header. This option should only be used
+  for the boot image.
+  This will also cause the image to be written to the specified
+  CPU's address space. If not specified, the default is CPU 0.
+ - Forces the file to be treated as a raw image. This can be
+  used to specify the load address of ELF files.
+
+All values are parsed using the standard QemuOps parsing. This allows the user
+to specify any values in any format supported. By default the values
+will be parsed as decimal. To use hex values the user should prefix the number
+with a '0x'.
+
+An example of loading an ELF file which CPU0 will boot is shown below:
+-device loader,file=./images/boot.elf,cpu-num=0
-- 
2.7.4

Re: [Qemu-devel] [PATCH v12 0/2] Add a generic loader

[Alistair Francis]

On Thu, Sep 29, 2016 at 5:09 PM, Alistair Francis
 wrote:
> On Thu, Sep 29, 2016 at 5:00 PM, Peter Maydell  
> wrote:
>> On 29 September 2016 at 02:25, Markus Armbruster  wrote:
>>> Patchew hiccup or a real problem?
>>
>> Looks like a hiccup to me.
>
> Same, it looks like it can't clone dtc.
>
> Thanks,
>
> Alistair
>
>>
>> Can you let me know if you're happy with the new commit messages?
>> If so I'll apply these to target-arm.next.

I just sent out a V13 which include Markus's updates. It doesn't touch
any code, just updated the comments in the c file and re-words the
value passing in the doc.

Thanks,

Alistair

>>
>> thanks
>> -- PMM
>>

[Qemu-devel] [PATCH v13 0/2] Add a generic loader

[Alistair Francis]

This work is based on the original work by Li Guang with extra
features added by Peter C and myself.

The idea of this loader is to allow the user to load multiple images
or values into QEMU at startup.

Memory values can be loaded like this: -device 
loader,addr=0xfd1a0104,data=0x800e,data-len=4

Images can be loaded like this: -device loader,file=./images/u-boot.elf,cpu=0

This can be useful and we use it a lot in Xilinx to load multiple images
into a machine at creation (ATF, Kernel and DTB for example).

Tested with the latest Xilinx ZynqMP machine, if I enable EL3 and EL2 I can
boot ATF through to u-boot using the loader to load the images.

It can also be used to set registers.

This patch series makes the load_elf() function more generic by not
requiring an architecture. It also adds new functions load_elf_as(),
load_uimage_as and load_image_targphys_as which allows custom
AddressSpaces when loading images.

V12:
 - All patches have been reviewed
 - Most patches have been merged
 - The commit message of the actual device patch has been updated to
   justify why it is a device.


Alistair Francis (2):
  generic-loader: Add a generic loader
  docs: Add a generic loader explanation document

 MAINTAINERS  |   6 ++
 docs/generic-loader.txt  |  84 
 hw/core/Makefile.objs|   2 +
 hw/core/generic-loader.c | 211 +++
 include/hw/core/generic-loader.h |  46 +
 5 files changed, 349 insertions(+)
 create mode 100644 docs/generic-loader.txt
 create mode 100644 hw/core/generic-loader.c
 create mode 100644 include/hw/core/generic-loader.h

-- 
2.7.4

Re: [Qemu-devel] [PULL 0/4] IDE patches

[Peter Maydell]

On 29 September 2016 at 13:15, John Snow  wrote:
> The following changes since commit c640f2849ee8775fe1bbd7a2772610aa77816f9f:
>
>   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into 
> staging (2016-09-28 23:02:56 +0100)
>
> are available in the git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to ca44141d5fb801dd5903102acefd0f2d8e8bb6a1:
>
>   ide: Fix memory leak in ide_register_restart_cb() (2016-09-29 15:50:29 
> -0400)
>
> 
>
> 


Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/virt: add 2.8 machine type

[Peter Maydell]

On 23 September 2016 at 07:41, Andrew Jones  wrote:
> Signed-off-by: Andrew Jones 
>
> ---
>
> This will conflict with Wei's PMU property series (which is why I
> asked him to add it to that series...), but anyway, it'll be a minor
> resolution to make.
> ---



Applied to target-arm.next, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v3 0/2] vmstateify a bunch of rare devices

[Peter Maydell]

On 27 September 2016 at 05:02, Dr. David Alan Gilbert (git)
 wrote:
> From: "Dr. David Alan Gilbert" 
>
> Two more register_savevm users converted to vmstate.
> This is split out from an earlier series where some of the others
> have gone in.
>
> Note I've not been able to test the changes (other than build); I've not
> got any model that uses them that successfully migrates even without the
> changes.
>
> Dave
>
> v3
>   A few fixes based on Peter's comments; mostly in converting what are
>   now booleans to be used as booleans in most places, avoiding &=, using false
>   and true.  Made 'timing' uint16_t rather than int16_t.

Applied to target-arm.next, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v12 2/2] docs: Add a generic loader explanation document

[Alistair Francis]

On Thu, Sep 29, 2016 at 2:24 AM, Markus Armbruster  wrote:
> Alistair Francis  writes:
>
>> Signed-off-by: Alistair Francis 
>> Reviewed-by: Peter Maydell 
>> ---
>> V11:
>>  - Fix corrections
>> V10:
>>  - Split the data loading and PC setting
>> V9:
>>  - Clarify the image loading options
>> V8:
>>  - Improve documentation
>> V6:
>>  - Fixup documentation
>> V4:
>>  - Re-write to be more comprehensive
>>
>>  docs/generic-loader.txt | 81 
>> +
>>  1 file changed, 81 insertions(+)
>>  create mode 100644 docs/generic-loader.txt
>>
>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>> new file mode 100644
>> index 000..d1f8ce3
>> --- /dev/null
>> +++ b/docs/generic-loader.txt
>> @@ -0,0 +1,81 @@
>> +Copyright (c) 2016 Xilinx Inc.
>> +
>> +This work is licensed under the terms of the GNU GPL, version 2 or later.  
>> See
>> +the COPYING file in the top-level directory.
>> +
>> +
>> +The 'loader' device allows the user to load multiple images or values into
>> +QEMU at startup.
>> +
>> +Loading Data into Memory Values
>> +-
>> +The loader device allows memory values to be set from the command line. This
>> +can be done by following the syntax below:
>> +
>> + -device loader,addr=,data=,data-len=
>> +   [,data-be=][,cpu-num=]
>> +
>> +  - The address to store the data in.
>> +  - The value to be written to the address. The maximum size 
>> of
>> +  the data is 8 bytes.
>> +  - The length of the data in bytes. This argument must be
>> +  included if the data argument is.
>> +   - Set to true if the data to be stored on the guest should 
>> be
>> +  written as big endian data. The default is to write little
>> +  endian data.
>> +   - The number of the CPU's address space where the data 
>> should
>> +  be loaded. If not specified the address space of the first
>> +  CPU is used.
>> +
>> +For all values both hex and decimal values are allowed. By default the 
>> values
>> +will be parsed as decimal. To use hex values the user should prefix the 
>> number
>> +with a '0x'.
>
> Unless you bypassed QemuOpts number parsing somehow, octal works as
> well.  In case you did bypass: don't!  Command line consistency matters.
> Follow-up patch reverting the bypass would be required.
>
> Not sure we want to document QemuOpts number syntax everywhere we
> explain how a certain feature uses the command line.  A pointer to the
> canonical place could be better.  Anyway, not something that needs
> fixing before we commit.

I didn't bypass it, octal should work as well. I have clarified that a
bit in the doc.

>
>> +
>> +An example of loading value 0x800e to address 0xfd1a0104 is:
>> +-device loader,addr=0xfd1a0104,data=0x800e,data-len=4
>> +
>> +Setting a CPU's Program Counter
>> +-
>> +The loader device allows the CPU's PC to be set from the command line. This
>> +can be done by following the syntax below:
>> +
>> + -device loader,addr=,cpu-num=
>> +
>> +  - The value to use as the CPU's PC.
>> +   - The number of the CPU whose PC should be set to the
>> +  specified value.
>> +
>> +For all values both hex and decimal values are allowed. By default the 
>> values
>> +will be parsed as decimal. To use hex values the user should prefix the 
>> number
>> +with a '0x'.
>> +
>> +An example of setting CPU 0's PC to 0x8000 is:
>> +-device loader,addr=0x8000,cpu-num=0
>> +
>> +Loading Files
>> +-
>> +The loader device also allows files to be loaded into memory. This can be 
>> done
>> +similarly to setting memory values. The syntax is shown below:
>> +
>> +-device 
>> loader,file=[,addr=][,cpu-num=][,force-raw=]
>> +
>> +  - A file to be loaded into memory
>> +  - The addr in memory that the file should be loaded. This is
>> +  ignored if you are using an ELF (unless force-raw is 
>> true).
>> +  This is required if you aren't loading an ELF.
>> +   - This specifies the CPU that should be used. This is an
>> +  optional argument and will cause the CPU's PC to be set to
>> +  where the image is stored or in the case of an ELF file to
>> +  the value in the header. This option should only be used
>> +  for the boot image.
>> +  This will also cause the image to be written to the 
>> specified
>> +  CPU's address space. If not specified, the default is CPU 
>> 0.
>
> Using @cpu-num both for further specifying the meaning of @addr and for
> setting that CPU's PC is awkward.  Are you sure there will never be a
> use case where you need to specify the CPU without also setting 

[Qemu-devel] [PATCH v13 1/2] generic-loader: Add a generic loader

[Alistair Francis]

Add a generic loader to QEMU which can be used to load images or set
memory values.

Internally inside QEMU this is a device. It is a strange device that
provides no hardware interface but allows QEMU to monkey patch memory
specified when it is created. To be able to do this it has a reset
callback that does the memory operations.

This device allows the user to monkey patch memory. To be able to do
this it needs a backend to manage the datas, the same as other
memory-related devices. In this case as the backend is so trivial we
have merged it with the frontend instead of creating and maintaining a
seperate backend.

Signed-off-by: Alistair Francis 
Reviewed-by: Peter Maydell 
Acked-by: Markus Armbruster 
---
V13:
 - Include the commit message in the c file.
V12:
 - Update commit message to justify using -device
V11:
 - Small corrections
 - Don't check for !data as writing a value of 0 is valid.
V10:
 - Split out the PC setting and data loading
V9:
 - Fix error messages
 - Updated some incorrect logic
 - Add address space loading support for all image types
 - Explain why the reset is manually registered
V8:
 - Code corrections
 - Rebase
V7:
 - Rebase
V6:
 - Add error checking
V5:
 - Rebase
V4:
 - Allow the loader to work with every architecture
 - Move the file to hw/core
 - Increase the maximum number of CPUs
 - Make the CPU operations conditional
 - Convert the cpu option to cpu-num
 - Require the user to specify endianess
V3:
 - Pass the ram_size to load_image_targphys()
V2:
 - Add maintainers entry
 - Perform bounds checking
 - Register and unregister the reset in the realise/unrealise
Changes since RFC:
 - Add BE support

 MAINTAINERS  |   6 ++
 hw/core/Makefile.objs|   2 +
 hw/core/generic-loader.c | 211 +++
 include/hw/core/generic-loader.h |  46 +
 4 files changed, 265 insertions(+)
 create mode 100644 hw/core/generic-loader.c
 create mode 100644 include/hw/core/generic-loader.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f3c1f7f..9ecaaa5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1013,6 +1013,12 @@ M: Dmitry Fleytman 
 S: Maintained
 F: hw/net/e1000e*
 
+Generic Loader
+M: Alistair Francis 
+S: Maintained
+F: hw/core/generic-loader.c
+F: include/hw/core/generic-loader.h
+
 Subsystems
 --
 Audio
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index cfd4840..939c94e 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -17,3 +17,5 @@ common-obj-$(CONFIG_SOFTMMU) += loader.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
 common-obj-$(CONFIG_SOFTMMU) += register.o
 common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
+
+obj-$(CONFIG_SOFTMMU) += generic-loader.o
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
new file mode 100644
index 000..79ab6df
--- /dev/null
+++ b/hw/core/generic-loader.c
@@ -0,0 +1,211 @@
+/*
+ * Generic Loader
+ *
+ * Copyright (C) 2014 Li Guang
+ * Copyright (C) 2016 Xilinx Inc.
+ * Written by Li Guang 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ *
+ */
+
+/*
+ * Internally inside QEMU this is a device. It is a strange device that
+ * provides no hardware interface but allows QEMU to monkey patch memory
+ * specified when it is created. To be able to do this it has a reset
+ * callback that does the memory operations.
+
+ * This device allows the user to monkey patch memory. To be able to do
+ * this it needs a backend to manage the datas, the same as other
+ * memory-related devices. In this case as the backend is so trivial we
+ * have merged it with the frontend instead of creating and maintaining a
+ * seperate backend.
+ */
+
+#include "qemu/osdep.h"
+#include "qom/cpu.h"
+#include "hw/sysbus.h"
+#include "sysemu/dma.h"
+#include "hw/loader.h"
+#include "qapi/error.h"
+#include "hw/core/generic-loader.h"
+
+#define CPU_NONE 0x
+
+static void generic_loader_reset(void *opaque)
+{
+GenericLoaderState *s = GENERIC_LOADER(opaque);
+
+if (s->set_pc) {
+CPUClass *cc = CPU_GET_CLASS(s->cpu);
+cpu_reset(s->cpu);
+if (cc) {
+cc->set_pc(s->cpu, s->addr);
+}
+}
+
+if (s->data_len) {
+assert(s->data_len < sizeof(s->data));
+dma_memory_write(s->cpu->as, s->addr, >data, s->data_len);
+}
+}
+
+static void generic_loader_realize(DeviceState 

Re: [Qemu-devel] [PATCH v12 0/2] Add a generic loader

[Peter Maydell]

On 29 September 2016 at 17:09, Alistair Francis
 wrote:
> On Thu, Sep 29, 2016 at 5:00 PM, Peter Maydell  
> wrote:
>> On 29 September 2016 at 02:25, Markus Armbruster  wrote:
>>> Patchew hiccup or a real problem?
>>
>> Looks like a hiccup to me.
>
> Same, it looks like it can't clone dtc

I think it cloned dtc but then hung after that; either way,
nothing to do with these patches.

-- PMM

Re: [Qemu-devel] [PATCH v12 1/2] generic-loader: Add a generic loader

[Alistair Francis]

On Thu, Sep 29, 2016 at 2:32 AM, Markus Armbruster  wrote:
> Alistair Francis  writes:
>
>> Add a generic loader to QEMU which can be used to load images or set
>> memory values.
>>
>> Internally inside QEMU this is a device. It is a strange device that
>> provides no hardware interface but allows QEMU to monkey patch memory
>> specified when it is created. To be able to do this it has a reset
>> callback that does the memory operations.
>>
>> This device allows the user to monkey patch memory. To be able to do
>> this it needs a backend to manage the datas, the same as other
>> memory-related devices. In this case as the backend is so trivial we
>> have merged it with the frontend instead of creating and maintaining a
>> seperate backend.
>
> Works for me.

Great!

>
>> Signed-off-by: Alistair Francis 
>> Reviewed-by: Peter Maydell 
> [...]
>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
>> new file mode 100644
>> index 000..fc2fea7
>> --- /dev/null
>> +++ b/hw/core/generic-loader.c
>> @@ -0,0 +1,197 @@
>> +/*
>> + * Generic Loader
>> + *
>> + * Copyright (C) 2014 Li Guang
>> + * Copyright (C) 2016 Xilinx Inc.
>> + * Written by Li Guang 
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful, but 
>> WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
>> + * for more details.
>> + */
>
> The text you added to the commit message would make a lovely comment
> here.  Please add it.

Adding it in.

>
>> +
>> +#include "qemu/osdep.h"
>> +#include "qom/cpu.h"
>> +#include "hw/sysbus.h"
>> +#include "sysemu/dma.h"
>> +#include "hw/loader.h"
>> +#include "qapi/error.h"
>> +#include "hw/core/generic-loader.h"
> [...]
>
> Thank you very much for processing my much-too-late design review
> graciously.

No worries

>
> Acked-by: Markus Armbruster 

Thanks!

Alistair

>

Re: [Qemu-devel] [PATCH v2] hw/arm: Fix Integrator/CM initialization

[Peter Maydell]

On 25 September 2016 at 13:35, Jakub Jermář  wrote:
>
> Initialization of a class instance cannot depend on its own properties
> as these are not yet set.  Move parts of integratorcm_init() that depend
> on the "memsz" property to the newly added integratorcm_realize().
>
> This fixes: https://bugs.launchpad.net/qemu/+bug/1624726
>
> Signed-off-by: Jakub Jermar 
> ---
>  hw/arm/integratorcp.c | 35 +--
>  1 file changed, 21 insertions(+), 14 deletions(-)

Applied to target-arm.next, thanks.

(PS: if you need to send patches in future, sending them inline
rather than as attachments works better with some of our patch
handling tools.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v12 0/2] Add a generic loader

[Alistair Francis]

On Thu, Sep 29, 2016 at 5:00 PM, Peter Maydell  wrote:
> On 29 September 2016 at 02:25, Markus Armbruster  wrote:
>> Patchew hiccup or a real problem?
>
> Looks like a hiccup to me.

Same, it looks like it can't clone dtc.

Thanks,

Alistair

>
> Can you let me know if you're happy with the new commit messages?
> If so I'll apply these to target-arm.next.
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v1 1/1] cadence_gem: Fix priority queue out of bounds access

[Peter Maydell]

On 26 September 2016 at 10:49, Alistair Francis
 wrote:
> There was an error with some of the register implementation assuming
> there are 16 priority queues supported when the IP only supports 8. This
> patch corrects the registers to only support 8 queues.
>
> Signed-off-by: Alistair Francis 
> Reported-by: Paolo Bonzini 
> ---
> Thanks to Paolo for pointing this out.
>
>  hw/net/cadence_gem.c | 22 --
>  1 file changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 8618e7a..7915732 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -147,25 +147,19 @@
>  #define GEM_INT_Q1_MASK (0x0640 / 4)
>
>  #define GEM_TRANSMIT_Q1_PTR (0x0440 / 4)
> -#define GEM_TRANSMIT_Q15_PTR(GEM_TRANSMIT_Q1_PTR + 14)
> +#define GEM_TRANSMIT_Q7_PTR (GEM_TRANSMIT_Q1_PTR + 6)

8 queues but they're numbered one to ... seven ??

thanks
-- PMM

[Qemu-devel] [PATCH v2] CODING_STYLE: Fix a typo ("have" vs. "has")

[Jonathan Neuschäfer]

Signed-off-by: Jonathan Neuschäfer 

---
v2:
- Preserve the poetic sound of "Many a flamewar", which Peter Maydell
  pointed out.
---
 CODING_STYLE | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CODING_STYLE b/CODING_STYLE
index e7fde15..f53180b 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -9,7 +9,7 @@ patches before submitting.
 Of course, the most important aspect in any coding style is whitespace.
 Crusty old coders who have trouble spotting the glasses on their noses
 can tell the difference between a tab and eight spaces from a distance
-of approximately fifteen parsecs.  Many a flamewar have been fought and
+of approximately fifteen parsecs.  Many a flamewar has been fought and
 lost on this issue.
 
 QEMU indents are four spaces.  Tabs are never used, except in Makefiles
-- 
2.9.3

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/2] mainstone: Fix incorrect and missed out keypad mappings.

[Peter Maydell]

On 28 September 2016 at 04:43, Vijay Kumar B  wrote:
> Resending, since I missed qemu-devel in the previous submission. Sorry
> about that.
>
> This patch series fixes the key mappings for the mainstone board. The
> incorrect mappings renders the console unusable when used with the
> Linux kernel's fbcon driver.
>
> Vijay Kumar B (2):
>   Fix incorrect key mapping for Enter key.
>   Add mapping for dot, slash and backspace.
>
>  hw/arm/mainstone.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)

Applied to target-arm.next, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v12 0/2] Add a generic loader

[Peter Maydell]

On 29 September 2016 at 02:25, Markus Armbruster  wrote:
> Patchew hiccup or a real problem?

Looks like a hiccup to me.

Can you let me know if you're happy with the new commit messages?
If so I'll apply these to target-arm.next.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 6/8] STM32F205: Connect the ADC devices

[Peter Maydell]

On 24 September 2016 at 12:20, Alistair Francis  wrote:
> Connect the ADC devices to the STM32F205 SoC.
>
> Signed-off-by: Alistair Francis 
> ---
>
>  #define FLASH_BASE_ADDRESS 0x0800
>  #define FLASH_SIZE (1024 * 1024)
> @@ -52,6 +55,9 @@ typedef struct STM32F205State {
>  STM32F2XXSyscfgState syscfg;
>  STM32F2XXUsartState usart[STM_NUM_USARTS];
>  STM32F2XXTimerState timer[STM_NUM_TIMERS];
> +STM32F2XXADCState adc[STM_NUM_ADCS];
> +
> +qemu_or_irq *adc_irqs;

Seems mildly inconsistent that all the other devices
"owned" by this SoC are embedded in the struct but this
one is a pointer (and so initialized via object_new()
rather than object_initialized()).

That's not a big deal though, so I've applied this
series to target-arm.next; you can change the above
in a followup patch, or not, as you choose.

>  } STM32F205State;
>
>  #endif
> --
> 2.7.4

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 5/8] irq: Add a new irq device that allows the ORing of lines

[Peter Maydell]

On 24 September 2016 at 12:20, Alistair Francis  wrote:
> Signed-off-by: Alistair Francis 
> ---
> As the migration framework is not included in user mode this needs to be a
> new file.
>
> V8:
>  - Use the standard qdev_init_gpio_in() function
> V7:
>  - Use the standard QEMU init/realise functions
> V6:
>  - Make the OR IRQ device a TYPE_DEVICE
>  - Add vmstate

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH] CODING_STYLE: Fix a typo ("Many a flamewar")

[Jonathan Neuschäfer]

On Thu, Sep 29, 2016 at 03:14:45PM -0700, Peter Maydell wrote:
> On 29 September 2016 at 13:46, Jonathan Neuschäfer
>  wrote:
> > Signed-off-by: Jonathan Neuschäfer 
> > ---
> >  CODING_STYLE | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/CODING_STYLE b/CODING_STYLE
> > index e7fde15..a03a994 100644
> > --- a/CODING_STYLE
> > +++ b/CODING_STYLE
> > @@ -9,7 +9,7 @@ patches before submitting.
> >  Of course, the most important aspect in any coding style is whitespace.
> >  Crusty old coders who have trouble spotting the glasses on their noses
> >  can tell the difference between a tab and eight spaces from a distance
> > -of approximately fifteen parsecs.  Many a flamewar have been fought and
> > +of approximately fifteen parsecs.  Many flamewars have been fought and
> >  lost on this issue.
> 
> Should be "Many a flamewar has been fought and lost".
> (This is slightly poetic phrasing but it has a nice ring to it;
> the complement does have to be singular, not plural, though.)

Thanks for pointing that out; I didn't know the phrase. I'll update my
patch.


Jonathan Neuschäfer


signature.asc
Description: PGP signature

Re: [Qemu-devel] [RFC 0/4] QOM class properties - do we need them?

[David Gibson]

On Thu, Sep 29, 2016 at 12:16:48PM +0200, Andreas Färber wrote:
> Am 29.09.2016 um 02:16 schrieb David Gibson:
> > is there really any value to supporting the "class"
> > properties in addition to the "instance" properties?
> 
> Yes, it makes enumerating available properties easier by not requiring
> to instantiate a new instance for printing, e.g., ",help"
> information.

A good point.  I shall try to be on the lookup for opportunities to
change existing properties to class properties.

-- 
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: [Qemu-devel] [RFC 0/4] QOM class properties - do we need them?

[David Gibson]

On Thu, Sep 29, 2016 at 12:23:41PM +0200, Andreas Färber wrote:
> Am 29.09.2016 um 12:21 schrieb Daniel P. Berrange:
> > On Thu, Sep 29, 2016 at 12:12:32PM +0200, Andreas Färber wrote:
> >> Am 29.09.2016 um 10:14 schrieb Daniel P. Berrange:
> >>> Practically all instances properties should become class properties
> >>> as its going to save wasting memory once most are converted.
> >>
> >> Not all, but most. child<> properties were the reason to have properties
> >> on the instance.
> > 
> > That's why I said "Practically all", instead of just "all" :-)
> 
> To me as non-native speaker "practically" is the opposite of
> "theoretically". :)

Heh, more English corner cases.  "Theoretically" and "in theory" mean
the same thing, but "practically" and "in practice" don't.  Or.. they
do in theory, but not in practice :p.  For bonus confusion, saying
"practically speaking" would, in this context, mean what you expected,
but "practically" on its own doesn't - it means basically the same as
"almost" or "nearly".

-- 
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: [Qemu-devel] [RFC 0/4] QOM class properties - do we need them?

[David Gibson]

On Thu, Sep 29, 2016 at 09:14:16AM +0100, Daniel P. Berrange wrote:
> On Thu, Sep 29, 2016 at 10:16:41AM +1000, David Gibson wrote:
> > QOM has the concept of both "object class" properties and "object
> > instance" properties.
> > 
> > The accessor functions installed for the rarely-used class properties
> > still take an Object *, so the *value* of such properties is still
> > per-instance; it's just the *existence* (and type) of the property
> > that is per-class.
> 
> Yes, of course. This is the whole point of class properties. It avoids
> allocating the same ObjectProperty struct against every object instance
> which wastes massive amounts of memory in scenarios where there are lots
> of instances created.

Ah, that makes sense.

> > Of course, that's also true in practice for the great majority of
> > "instance" properties, because they're created identically and
> > unconditionally for every instance from the per-class instance_init
> > hook.
> > 
> > This also means that the (unused) object_class_property_add_*_ptr()
> > functions don't make a lot of sense, since they require a fixed
> > pointer which means the value of such a property would only be
> > per-class.
> > 
> > Given that, is there really any value to supporting the "class"
> > properties in addition to the "instance" properties?  This series is
> > an RFC which removes all support for class properties, changing the
> > few existing users to instance properties instead.
> > 
> > Alternatively, if we *don't* want to remove class properties, should
> > we instead be trying to convert the many, many "instance" properties
> > whose existence is actually per-class to be class properties?
> 
> Practically all instances properties should become class properties
> as its going to save wasting memory once most are converted.

Heh, ok.  Well, I'll keep that in mind when I'm adding properties in
future.  I wonder if there's a way we can better get the word out that
this is how properties should usually be done.

That said.. I'm still thinking we should remove
object_class_property_add_*_ptr().  Those take an actual pointer to
the value, meaning that it can't have different values per-instance.
These only create read-only properties, so they're not actually
dangerous, but they really don't seem very useful.

-- 
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: [Qemu-devel] [PATCH 2/2] target-ppc/kvm: Enable transactional memory on POWER8 with KVM-HV, too

[David Gibson]

On Thu, Sep 29, 2016 at 12:48:07PM +0200, Thomas Huth wrote:
> Transactional memory is also supported on POWER8 KVM-HV if the
> KVM_CAP_PPC_HTM is not available in the kernel yet, so add a hack
> to allow TM here, too.
> 
> Signed-off-by: Thomas Huth 

Applied to ppc-for-2.8.

> ---
>  hw/ppc/spapr.c   | 2 +-
>  target-ppc/kvm.c | 9 -
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fc37145..2f4e818 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -584,7 +584,7 @@ static void spapr_populate_pa_features(CPUPPCState *env, 
> void *fdt, int offset)
>   */
>  pa_features[3] |= 0x20;
>  }
> -if (kvmppc_has_cap_htm()) {
> +if (kvmppc_has_cap_htm() && pa_size > 24) {

It looks like this is an unrelated fix to the original TM test.  I'll
fold it into the original patch.

>  pa_features[24] |= 0x80;/* Transactional memory support */
>  }
>  
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index dca50bc..e990cb7 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -567,11 +567,18 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  
>  idle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, kvm_kick_cpu, cpu);
>  
> -/* Some targets support access to KVM's guest TLB. */
>  switch (cenv->mmu_model) {
>  case POWERPC_MMU_BOOKE206:
> +/* This target supports access to KVM's guest TLB */
>  ret = kvm_booke206_tlb_init(cpu);
>  break;
> +case POWERPC_MMU_2_07:
> +if (!cap_htm && !kvmppc_is_pr(cs->kvm_state)) {
> +/* KVM-HV has transactional memory on POWER8 also without the
> + * KVM_CAP_PPC_HTM extension, so enable it here instead. */
> +cap_htm = true;
> +}
> +break;
>  default:
>  break;
>  }

-- 
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: [Qemu-devel] [PATCH 1/2] target-ppc/kvm: Add a wrapper function to check for KVM-PR

[David Gibson]

On Thu, Sep 29, 2016 at 12:48:06PM +0200, Thomas Huth wrote:
> It makes more sense if we have a proper function to check
> for KVM-PR than to check for the GET_PVINFO extension all
> over the place.
> 
> Signed-off-by: Thomas Huth 

Applied to ppc-for-2.8.  I expanded your comment to say this should
only be used for fallback tests, though.

> ---
>  target-ppc/kvm.c | 23 +--
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index e9a9faf..dca50bc 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -102,6 +102,13 @@ static void kvm_kick_cpu(void *opaque)
>  qemu_cpu_kick(CPU(cpu));
>  }
>  
> +/* Check whether we are running with KVM-PR (instead of KVM-HV) */
> +static bool kvmppc_is_pr(KVMState *ks)
> +{
> +/* Assume KVM-PR if the GET_PVINFO capability is available */
> +return kvm_check_extension(ks, KVM_CAP_PPC_GET_PVINFO) != 0;
> +}
> +
>  static int kvm_ppc_register_host_cpu_type(void);
>  
>  int kvm_arch_init(MachineState *ms, KVMState *s)
> @@ -223,10 +230,9 @@ static void kvm_get_fallback_smmu_info(PowerPCCPU *cpu,
>   *
>   * For that to work we make a few assumptions:
>   *
> - * - If KVM_CAP_PPC_GET_PVINFO is supported we are running "PR"
> - *   KVM which only supports 4K and 16M pages, but supports them
> - *   regardless of the backing store characteritics. We also don't
> - *   support 1T segments.
> + * - Check whether we are running "PR" KVM which only supports 4K
> + *   and 16M pages, but supports them regardless of the backing
> + *   store characteritics. We also don't support 1T segments.
>   *
>   *   This is safe as if HV KVM ever supports that capability or PR
>   *   KVM grows supports for more page/segment sizes, those versions
> @@ -241,7 +247,7 @@ static void kvm_get_fallback_smmu_info(PowerPCCPU *cpu,
>   *   implements KVM_CAP_PPC_GET_SMMU_INFO and thus doesn't hit
>   *   this fallback.
>   */
> -if (kvm_check_extension(cs->kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
> +if (kvmppc_is_pr(cs->kvm_state)) {
>  /* No flags */
>  info->flags = 0;
>  info->slb_size = 64;
> @@ -2270,11 +2276,8 @@ int kvmppc_reset_htab(int shift_hint)
>  
>  /* We have a kernel that predates the htab reset calls.  For PR
>   * KVM, we need to allocate the htab ourselves, for an HV KVM of
> - * this era, it has allocated a 16MB fixed size hash table
> - * already.  Kernels of this era have the GET_PVINFO capability
> - * only on PR, so we use this hack to determine the right
> - * answer */
> -if (kvm_check_extension(kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
> + * this era, it has allocated a 16MB fixed size hash table already. */
> +if (kvmppc_is_pr(kvm_state)) {
>  /* PR - tell caller to allocate htab */
>  return 0;
>  } else {

-- 
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: [Qemu-devel] [PATCH] target-ppc: fix vmx instruction type/type2

[David Gibson]

On Thu, Sep 29, 2016 at 03:52:37PM +0530, Nikunj A Dadhania wrote:
> A few of the new instructions added inadvertently changed the type of
> old instruction(PPC_ALTIVEC) to PPC2_ALTIVEC_207 in the dual form
> declaration.
> 
> commit: b5d569a1 (target-ppc: add vector extract instructions)
> commit: e7b1e06f (target-ppc: add vector insert instructions)
> commit: 3aa56a19 (target-ppc: add vector compare not equal instructions)
> 
> New ISA 3.0 instructions added:
> vextractub PPC_NONE PPC2_ISA300
> vextractuh PPC_NONE PPC2_ISA300
> vextractuw PPC_NONE PPC2_ISA300
> vinsertb   PPC_NONE PPC2_ISA300
> vinserth   PPC_NONE PPC2_ISA300
> vinsertw   PPC_NONE PPC2_ISA300
> vcmpnebPPC_NONE PPC2_ISA300
> vcmpnehPPC_NONE PPC2_ISA300
> vcmpnewPPC_NONE PPC2_ISA300
> 
> Affected older instructions:
> vspltb PPC_ALTIVEC  PPC_NONE
> vsplth PPC_ALTIVEC  PPC_NONE
> vspltw PPC_ALTIVEC  PPC_NONE
> vspltisb   PPC_ALTIVEC  PPC_NONE
> vspltish   PPC_ALTIVEC  PPC_NONE
> vspltisw   PPC_ALTIVEC  PPC_NONE
> vcmpequb   PPC_ALTIVEC  PPC_NONE
> vcmpequh   PPC_ALTIVEC  PPC_NONE
> vcmpequw   PPC_ALTIVEC  PPC_NONE
> 
> Change the instruction type/type2 for the older instructions back to
> what it was(PPC_ALTIVEC).
> 
> CC: Rajalakshmi Srinivasaraghavan 
> Reported-by: Bharata B Rao 
> Signed-off-by: Nikunj A Dadhania 

Applied to ppc-for-2.8, thanks.

> ---
>  target-ppc/translate/vmx-impl.inc.c | 30 +++---
>  target-ppc/translate/vmx-ops.inc.c  | 18 +-
>  2 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/target-ppc/translate/vmx-impl.inc.c 
> b/target-ppc/translate/vmx-impl.inc.c
> index f646e85..25cd073 100644
> --- a/target-ppc/translate/vmx-impl.inc.c
> +++ b/target-ppc/translate/vmx-impl.inc.c
> @@ -514,11 +514,11 @@ GEN_VXRFORM(vcmpneb, 3, 0)
>  GEN_VXRFORM(vcmpneh, 3, 1)
>  GEN_VXRFORM(vcmpnew, 3, 2)
>  
> -GEN_VXRFORM_DUAL(vcmpequb, PPC_NONE, PPC2_ALTIVEC_207, \
> +GEN_VXRFORM_DUAL(vcmpequb, PPC_ALTIVEC, PPC_NONE, \
>   vcmpneb, PPC_NONE, PPC2_ISA300)
> -GEN_VXRFORM_DUAL(vcmpequh, PPC_NONE, PPC2_ALTIVEC_207, \
> +GEN_VXRFORM_DUAL(vcmpequh, PPC_ALTIVEC, PPC_NONE, \
>   vcmpneh, PPC_NONE, PPC2_ISA300)
> -GEN_VXRFORM_DUAL(vcmpequw, PPC_NONE, PPC2_ALTIVEC_207, \
> +GEN_VXRFORM_DUAL(vcmpequw, PPC_ALTIVEC, PPC_NONE, \
>   vcmpnew, PPC_NONE, PPC2_ISA300)
>  GEN_VXRFORM_DUAL(vcmpeqfp, PPC_ALTIVEC, PPC_NONE, \
>   vcmpequd, PPC_NONE, PPC2_ALTIVEC_207)
> @@ -712,18 +712,18 @@ GEN_VXFORM_UIMM_ENV(vcfux, 5, 12);
>  GEN_VXFORM_UIMM_ENV(vcfsx, 5, 13);
>  GEN_VXFORM_UIMM_ENV(vctuxs, 5, 14);
>  GEN_VXFORM_UIMM_ENV(vctsxs, 5, 15);
> -GEN_VXFORM_DUAL(vspltb, PPC_NONE, PPC2_ALTIVEC_207,
> -  vextractub, PPC_NONE, PPC2_ISA300);
> -GEN_VXFORM_DUAL(vsplth, PPC_NONE, PPC2_ALTIVEC_207,
> -  vextractuh, PPC_NONE, PPC2_ISA300);
> -GEN_VXFORM_DUAL(vspltw, PPC_NONE, PPC2_ALTIVEC_207,
> -  vextractuw, PPC_NONE, PPC2_ISA300);
> -GEN_VXFORM_DUAL(vspltisb, PPC_NONE, PPC2_ALTIVEC_207,
> -  vinsertb, PPC_NONE, PPC2_ISA300);
> -GEN_VXFORM_DUAL(vspltish, PPC_NONE, PPC2_ALTIVEC_207,
> -  vinserth, PPC_NONE, PPC2_ISA300);
> -GEN_VXFORM_DUAL(vspltisw, PPC_NONE, PPC2_ALTIVEC_207,
> -  vinsertw, PPC_NONE, PPC2_ISA300);
> +GEN_VXFORM_DUAL(vspltb, PPC_ALTIVEC, PPC_NONE,
> +vextractub, PPC_NONE, PPC2_ISA300);
> +GEN_VXFORM_DUAL(vsplth, PPC_ALTIVEC, PPC_NONE,
> +vextractuh, PPC_NONE, PPC2_ISA300);
> +GEN_VXFORM_DUAL(vspltw, PPC_ALTIVEC, PPC_NONE,
> +vextractuw, PPC_NONE, PPC2_ISA300);
> +GEN_VXFORM_DUAL(vspltisb, PPC_ALTIVEC, PPC_NONE,
> +vinsertb, PPC_NONE, PPC2_ISA300);
> +GEN_VXFORM_DUAL(vspltish, PPC_ALTIVEC, PPC_NONE,
> +vinserth, PPC_NONE, PPC2_ISA300);
> +GEN_VXFORM_DUAL(vspltisw, PPC_ALTIVEC, PPC_NONE,
> +vinsertw, PPC_NONE, PPC2_ISA300);
>  
>  static void gen_vsldoi(DisasContext *ctx)
>  {
> diff --git a/target-ppc/translate/vmx-ops.inc.c 
> b/target-ppc/translate/vmx-ops.inc.c
> index b63e33d..ac1dc9b 100644
> --- a/target-ppc/translate/vmx-ops.inc.c
> +++ b/target-ppc/translate/vmx-ops.inc.c
> @@ -194,26 +194,26 @@ GEN_VXRFORM_DUAL(vcmpeqfp, vcmpequd, 3, 3, PPC_ALTIVEC, 
> PPC_NONE)
>  GEN_VXRFORM(vcmpgefp, 3, 7)
>  GEN_VXRFORM_DUAL(vcmpgtfp, vcmpgtud, 3, 11, PPC_ALTIVEC, PPC_NONE)
>  GEN_VXRFORM_DUAL(vcmpbfp, vcmpgtsd, 3, 15, PPC_ALTIVEC, PPC_NONE)
> -GEN_VXRFORM_DUAL(vcmpequb, vcmpneb, 3, 0, PPC_NONE, PPC2_ALTIVEC_207)
> -GEN_VXRFORM_DUAL(vcmpequh, vcmpneh, 3, 1, PPC_NONE, PPC2_ALTIVEC_207)
> -GEN_VXRFORM_DUAL(vcmpequw, vcmpnew, 3, 2, PPC_NONE, 

Re: [Qemu-devel] [PATCH] bitmap: refine and move BITMAP_{FIRST/LAST}_WORD_MASK

[Wei Yang]

On Thu, Sep 29, 2016 at 07:11:39PM +0300, Michael Tokarev wrote:
>05.03.2016 16:47, Wei Yang wrote:
>>According to linux kernel commit <89c1e79eb30> ("linux/bitmap.h: improve
>>BITMAP_{LAST,FIRST}_WORD_MASK"), these two macro could be improved.
>>
>>This patch takes this change and also move them all in header file.
>
>Applied to -trivial, thank you!
>

Thanks :-) Have a good day~

>/mjt

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [Qemu-ppc] How to add my implementation of the fmadds instruction to QEMU

[Programmingkid]

On Sep 29, 2016, at 6:36 PM, Alex Bennée wrote:

> 
> Programmingkid  writes:
> 
>> On Sep 29, 2016, at 2:19 PM, Alex Bennée wrote:
>> 
>>> 
>>> Programmingkid  writes:
>>> 
 On Sep 29, 2016, at 12:17 AM, David Gibson wrote:
 
> On Tue, Sep 27, 2016 at 09:58:02AM -0700, Peter Maydell wrote:
>> On 27 September 2016 at 09:51, G 3  wrote:
>>> The problem with your reasoning is you assume this instruction has to be
>>> 100% correctly implemented. That every single "corner-case" has to be
>>> accounted for.
>> 
>> For upstream QEMU we've already made this design decision --
>> emulation accuracy comes first, and speed is secondary.
>> That's why we implement this the way we do.
> 
> I think there is a way you could get both speed and accuracy, but it's
> a huge project:
> 
> You'd need to add full float awareness to TCG - so floating point TCG
> values and floating point operations as tcp micro-ops, defined
> according to IEEE semantics.  Then you'd need to rewrite the TCG
> frontends in terms of those new ops, at least for target CPUs close
> enough to IEEE semantics for that to work.  And you'd need to rewrite
> the TCG backends to implement those fp ops in terms of host cpu fp
> instructions .. at least when the host has fp behaviour close enough
> to IEEE to make that work, with a fallback to soft float when that's
> not the case.
 
 Interesting idea. Do you think we would see a large enough increase in 
 speed
 to justify this project?
>>> 
>>> It really depends on workload. If you want to run lots of encoding/audio
>>> workloads in TCG guests it is certainly something that could be
>>> improved.
>>> 
>>> As others have pointed out however it is a fairly big project.
>>> 
>>> --
>>> Alex Bennée
>> 
>> Alex Bennée? I was just watching your KVM video about MTTCG! Small world.
>> 
>> I so want audio to play correctly in a PowerPC Mac OS guest. So this
>> project might be necessary.
>> 
>> If it is a fairly big project, then I will need to map it out some more.
>> I've made a mind map of what I know so far. It is attached to this email.
>> Let me know if you can think of anything to add.
>> 
>> http://i.imgur.com/MYkiKGx.png
> 
> While I appreciate your target is PPC I think if you are going to
> suggest any core floating point TCGOps you will need to survey the
> behaviour of the FPUs on all (or at least the most common) TCG targets
> and go for instructions that behave the same across a broad range of
> targets.
> 
> I think if we were to introduce this into the code base we would need to
> have a decent range of test cases. I'm talking about making sure we
> exercise the whole range of behaviour:
> 
>  - min/max rounding behaviour
>  - handling of denormalisation
>  - signalling and non-signalling NaN behaviour
>  - exception generation
> 
> Testing is going to be very important for confidence.

Thank you very much for this list. I will see what I can come up with.

Re: [Qemu-devel] [Qemu-ppc] How to add my implementation of the fmadds instruction to QEMU

[Alex Bennée]

Programmingkid  writes:

> On Sep 29, 2016, at 2:19 PM, Alex Bennée wrote:
>
>>
>> Programmingkid  writes:
>>
>>> On Sep 29, 2016, at 12:17 AM, David Gibson wrote:
>>>
 On Tue, Sep 27, 2016 at 09:58:02AM -0700, Peter Maydell wrote:
> On 27 September 2016 at 09:51, G 3  wrote:
>> The problem with your reasoning is you assume this instruction has to be
>> 100% correctly implemented. That every single "corner-case" has to be
>> accounted for.
>
> For upstream QEMU we've already made this design decision --
> emulation accuracy comes first, and speed is secondary.
> That's why we implement this the way we do.

 I think there is a way you could get both speed and accuracy, but it's
 a huge project:

 You'd need to add full float awareness to TCG - so floating point TCG
 values and floating point operations as tcp micro-ops, defined
 according to IEEE semantics.  Then you'd need to rewrite the TCG
 frontends in terms of those new ops, at least for target CPUs close
 enough to IEEE semantics for that to work.  And you'd need to rewrite
 the TCG backends to implement those fp ops in terms of host cpu fp
 instructions .. at least when the host has fp behaviour close enough
 to IEEE to make that work, with a fallback to soft float when that's
 not the case.
>>>
>>> Interesting idea. Do you think we would see a large enough increase in speed
>>> to justify this project?
>>
>> It really depends on workload. If you want to run lots of encoding/audio
>> workloads in TCG guests it is certainly something that could be
>> improved.
>>
>> As others have pointed out however it is a fairly big project.
>>
>> --
>> Alex Bennée
>
> Alex Bennée? I was just watching your KVM video about MTTCG! Small world.
>
> I so want audio to play correctly in a PowerPC Mac OS guest. So this
> project might be necessary.
>
> If it is a fairly big project, then I will need to map it out some more.
> I've made a mind map of what I know so far. It is attached to this email.
> Let me know if you can think of anything to add.
>
> http://i.imgur.com/MYkiKGx.png

While I appreciate your target is PPC I think if you are going to
suggest any core floating point TCGOps you will need to survey the
behaviour of the FPUs on all (or at least the most common) TCG targets
and go for instructions that behave the same across a broad range of
targets.

I think if we were to introduce this into the code base we would need to
have a decent range of test cases. I'm talking about making sure we
exercise the whole range of behaviour:

  - min/max rounding behaviour
  - handling of denormalisation
  - signalling and non-signalling NaN behaviour
  - exception generation

Testing is going to be very important for confidence.

--
Alex Bennée

Re: [Qemu-devel] [PATCH] CODING_STYLE: Fix a typo ("Many a flamewar")

[Peter Maydell]

On 29 September 2016 at 13:46, Jonathan Neuschäfer
 wrote:
> Signed-off-by: Jonathan Neuschäfer 
> ---
>  CODING_STYLE | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index e7fde15..a03a994 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -9,7 +9,7 @@ patches before submitting.
>  Of course, the most important aspect in any coding style is whitespace.
>  Crusty old coders who have trouble spotting the glasses on their noses
>  can tell the difference between a tab and eight spaces from a distance
> -of approximately fifteen parsecs.  Many a flamewar have been fought and
> +of approximately fifteen parsecs.  Many flamewars have been fought and
>  lost on this issue.

Should be "Many a flamewar has been fought and lost".
(This is slightly poetic phrasing but it has a nice ring to it;
the complement does have to be singular, not plural, though.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] fdc: Add a floppy drive qdev

[John Snow]

On 09/05/2016 11:43 AM, Kevin Wolf wrote:

Floppy controllers automatically create two floppy drive devices in qdev
now. (They always created two drives, but managed them only internally.)



This appears to *actually* create and expose two drives by default, is 
this intentional?


Previously, we created "two drives", but only exposed ones for which 
drive->blk was true -- which was in practice only the first drive by 
default. The CMOS magic we'd expose would be either 0x40 or 0x50 (1.44MB 
and no drive or 2.88MB and no drive) -- now it's 0x55 (two 2.88MB drives.)


As it stands right now, both drives get made as type "AUTO". If 
drive->blk is present, we'll evaluate this to either 128, 144 or 288. 
We'll use a fallback type of 288 if no medium is present or we couldn't 
figure out what was in there.


Otherwise, we'll fall through to fd_revalidate's "No Drive Connected" 
segment which will more or less delete the drive.


This changes fdc-test and acpi-test which will need to be changed if 
this is really what you want.


--js


Signed-off-by: Kevin Wolf 
---
 hw/block/fdc.c | 152 +++--
 1 file changed, 115 insertions(+), 37 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 4164b31..d1e2339 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -60,6 +60,8 @@
 #define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)

 typedef struct FDCtrl FDCtrl;
+typedef struct FDrive FDrive;
+static FDrive *get_drv(FDCtrl *fdctrl, int unit);

 typedef struct FloppyBus {
 BusState bus;
@@ -180,7 +182,7 @@ typedef enum FDiskFlags {
 FDISK_DBL_SIDES  = 0x01,
 } FDiskFlags;

-typedef struct FDrive {
+struct FDrive {
 FDCtrl *fdctrl;
 BlockBackend *blk;
 /* Drive status */
@@ -201,7 +203,7 @@ typedef struct FDrive {
 uint8_t media_rate;   /* Data rate of medium*/

 bool media_validated; /* Have we validated the media? */
-} FDrive;
+};


 static FloppyDriveType get_fallback_drive_type(FDrive *drv);
@@ -466,6 +468,101 @@ static void fd_revalidate(FDrive *drv)
 }
 }

+static void fd_change_cb(void *opaque, bool load)
+{
+FDrive *drive = opaque;
+
+drive->media_changed = 1;
+drive->media_validated = false;
+fd_revalidate(drive);
+}
+
+static const BlockDevOps fd_block_ops = {
+.change_media_cb = fd_change_cb,
+};
+
+
+#define TYPE_FLOPPY_DRIVE "floppy"
+#define FLOPPY_DRIVE(obj) \
+ OBJECT_CHECK(FloppyDrive, (obj), TYPE_FLOPPY_DRIVE)
+
+typedef struct FloppyDrive {
+DeviceState qdev;
+uint32_tunit;
+} FloppyDrive;
+
+static Property floppy_drive_properties[] = {
+DEFINE_PROP_UINT32("unit", FloppyDrive, unit, -1),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static int floppy_drive_init(DeviceState *qdev)
+{
+FloppyDrive *dev = FLOPPY_DRIVE(qdev);
+FloppyBus *bus = DO_UPCAST(FloppyBus, bus, dev->qdev.parent_bus);
+FDrive *drive;
+
+if (dev->unit == -1) {
+for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
+drive = get_drv(bus->fdc, dev->unit);
+if (!drive->blk) {
+break;
+}
+}
+}
+
+if (dev->unit >= MAX_FD) {
+error_report("Can't create floppy unit %d, bus supports only %d units",
+ dev->unit, MAX_FD);
+return -1;
+}
+
+/* TODO Check whether unit is in use */
+
+drive = get_drv(bus->fdc, dev->unit);
+drive->fdctrl = bus->fdc;
+
+if (drive->blk) {
+if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
+error_report("fdc doesn't support drive option werror");
+return -1;
+}
+if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
+error_report("fdc doesn't support drive option rerror");
+return -1;
+}
+} else {
+/* Anonymous BlockBackend for an empty drive */
+drive->blk = blk_new();
+}
+
+fd_init(drive);
+if (drive->blk) {
+blk_set_dev_ops(drive->blk, _block_ops, drive);
+pick_drive_type(drive);
+}
+fd_revalidate(drive);
+
+return 0;
+}
+
+static void floppy_drive_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *k = DEVICE_CLASS(klass);
+k->init = floppy_drive_init;
+set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
+k->bus_type = TYPE_FLOPPY_BUS;
+k->props = floppy_drive_properties;
+k->desc = "virtual floppy drive";
+}
+
+static const TypeInfo floppy_drive_info = {
+.name = TYPE_FLOPPY_DRIVE,
+.parent = TYPE_DEVICE,
+.instance_size = sizeof(FloppyDrive),
+.class_init = floppy_drive_class_init,
+};
+
 //
 /* Intel 82078 floppy disk controller emulation  */

@@ -1184,9 +1281,9 @@ static inline FDrive *drv3(FDCtrl *fdctrl)
 }
 #endif

-static FDrive *get_cur_drv(FDCtrl *fdctrl)
+static FDrive *get_drv(FDCtrl *fdctrl, int unit)
 {

Re: [Qemu-devel] [Qemu-ppc] How to add my implementation of the fmadds instruction to QEMU

[Programmingkid]

On Sep 29, 2016, at 2:19 PM, Alex Bennée wrote:

> 
> Programmingkid  writes:
> 
>> On Sep 29, 2016, at 12:17 AM, David Gibson wrote:
>> 
>>> On Tue, Sep 27, 2016 at 09:58:02AM -0700, Peter Maydell wrote:
 On 27 September 2016 at 09:51, G 3  wrote:
> The problem with your reasoning is you assume this instruction has to be
> 100% correctly implemented. That every single "corner-case" has to be
> accounted for.
 
 For upstream QEMU we've already made this design decision --
 emulation accuracy comes first, and speed is secondary.
 That's why we implement this the way we do.
>>> 
>>> I think there is a way you could get both speed and accuracy, but it's
>>> a huge project:
>>> 
>>> You'd need to add full float awareness to TCG - so floating point TCG
>>> values and floating point operations as tcp micro-ops, defined
>>> according to IEEE semantics.  Then you'd need to rewrite the TCG
>>> frontends in terms of those new ops, at least for target CPUs close
>>> enough to IEEE semantics for that to work.  And you'd need to rewrite
>>> the TCG backends to implement those fp ops in terms of host cpu fp
>>> instructions .. at least when the host has fp behaviour close enough
>>> to IEEE to make that work, with a fallback to soft float when that's
>>> not the case.
>> 
>> Interesting idea. Do you think we would see a large enough increase in speed
>> to justify this project?
> 
> It really depends on workload. If you want to run lots of encoding/audio
> workloads in TCG guests it is certainly something that could be
> improved.
> 
> As others have pointed out however it is a fairly big project.
> 
> --
> Alex Bennée

Alex Bennée? I was just watching your KVM video about MTTCG! Small world. 

I so want audio to play correctly in a PowerPC Mac OS guest. So this
project might be necessary. 

If it is a fairly big project, then I will need to map it out some more.
I've made a mind map of what I know so far. It is attached to this email.
Let me know if you can think of anything to add.

http://i.imgur.com/MYkiKGx.png

[Qemu-devel] [ANNOUNCE] QEMU 2.6.2 Stable released

[Michael Roth]

Hi everyone,

I am pleased to announce that the QEMU v2.6.2 stable release is now
available:

  http://wiki.qemu.org/download/qemu-2.6.2.tar.bz2

v2.6.2 is now tagged in the official qemu.git repository,
and the stable-2.6 branch has been updated accordingly:

  http://git.qemu.org/?p=qemu.git;a=shortlog;h=refs/heads/stable-2.6

This is a fairly small update to 2.6.1 that addresses:

  - a security issue when using virtfs/virtio-9p which can lead to a guest
gaining unintended access to the host filesystem,
  - a regression introduced into 2.6.1 which can result in live migration
failing when virtio devices have pending entries in their virtqueues,
  - and various issues related to VNC and emulated network/storage devices,
among other things.

Users of QEMU 2.6.1 should upgrade accordingly.

Thank you to everyone involved!

CHANGELOG:

529d45e: Update version for 2.6.2 release (Michael Roth)
1b5520a: s390x/css: handle cssid 255 correctly (Cornelia Huck)
9ea7a46: ahci: clear aiocb in ncq_cb (John Snow)
1c57ced: vnc: fix incorrect checking condition when updating client (Gonglei)
98b8129: vnc-enc-tight: fix off-by-one bug (Herongguang (Stephen))
8ef7abe: vnc: make sure we finish disconnect (Gerd Hoffmann)
14713d6: vnc: don't crash getting server info if lsock is NULL (Daniel P. 
Berrange)
c5518b3: vnc: ensure connection sharing/limits is always configured (Daniel P. 
Berrange)
6be9ee1: vnc: fix crash when vnc_server_info_get has an error (Daniel P. 
Berrange)
2b13613: ui: avoid crash if vnc client disconnects with writes pending (Daniel 
P. Berrange)
6e18475: virtio-scsi: Don't abort when media is ejected (Fam Zheng)
469513b: scsi-disk: Cleaning up around tray open state (Fam Zheng)
12664c5: iothread: Stop threads before main() quits (Fam Zheng)
d904813: crypto: ensure XTS is only used with ciphers with 16 byte blocks 
(Daniel P. Berrange)
0751a60: scsi: mptconfig: fix misuse of MPTSAS_CONFIG_PACK (Paolo Bonzini)
5e39560: scsi: mptconfig: fix an assert expression (Prasad J Pandit)
da99530: vmw_pvscsi: check page count while initialising descriptor rings 
(Prasad J Pandit)
7aa7c25: scsi-disk: change disk serial length from 20 to 36 (Rony Weng)
b79239a: qemu-char: avoid segfault if user lacks of permisson of a given 
logfile (Lin Ma)
c5b64fb: scsi: pvscsi: limit process IO loop to ring size (Prasad J Pandit)
12be5cf: scsi: mptsas: use g_new0 to allocate MPTSASRequest object (Li Qiang)
5e2c6fe: 9pfs: fix potential segfault during walk (Greg Kurz)
b9ab2f6: vnc: fix qemu crash because of SIGSEGV (Gonglei)
44d28f2: virtio-balloon: discard virtqueue element on reset (Ladi Prosek)
1af2c3f: virtio: zero vq->inuse in virtio_reset() (Stefan Hajnoczi)
85d0a53: 9pfs: handle walk of ".." in the root directory (Greg Kurz)
b5191b2: 9pfs: forbid . and .. in file names (Greg Kurz)
917e9a9: 9pfs: forbid illegal path names (Greg Kurz)
cb3677c: net: vmxnet: use g_new for pkt initialisation (Li Qiang)
9306025: net: vmxnet: check IP header length (Li Qiang)
f216833: iscsi: pass SCSI status back for SG_IO (Vadim Rozenfeld)
ca86c04: virtio: decrement vq->inuse in virtqueue_discard() (Stefan Hajnoczi)
8e44714: virtio: recalculate vq->inuse after migration (Stefan Hajnoczi)
d00d89a: ui: fix refresh of VNC server surface (Daniel P. Berrange)
3b9717a: net: check fragment length during fragmentation (Prasad J Pandit)

[Qemu-devel] [PATCH v4 09/11] target-i386: x86_cpu_load_features() function

[Eduardo Habkost]

When probing for CPU model information, we need to reuse the code
that initializes CPUID fields, but not the remaining side-effects
of x86_cpu_realizefn(). Move that code to a separate function
that can be reused later.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 67 +++
 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b25657b..e099892 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2993,34 +2993,14 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
 env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
 }
 
-#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
-   (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
-   (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
-#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
- (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
- (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
-static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
+/* Load CPUID data based on configureured features
+ */
+static void x86_cpu_load_features(X86CPU *cpu, Error **errp)
 {
-CPUState *cs = CPU(dev);
-X86CPU *cpu = X86_CPU(dev);
-X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
 CPUX86State *env = >env;
-Error *local_err = NULL;
-static bool ht_warned;
 FeatureWord w;
 GList *l;
-
-if (xcc->kvm_required && !kvm_enabled()) {
-char *name = x86_cpu_class_get_model_name(xcc);
-error_setg(_err, "CPU model '%s' requires KVM", name);
-g_free(name);
-goto out;
-}
-
-if (cpu->apic_id == UNASSIGNED_APIC_ID) {
-error_setg(errp, "apic-id property was not initialized properly");
-return;
-}
+Error *local_err = NULL;
 
 /*TODO: cpu->host_features incorrectly overwrites features
  * set using "feat=on|off". Once we fix this, we can convert
@@ -3087,6 +3067,45 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 }
 
 x86_cpu_filter_features(cpu);
+
+out:
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+}
+}
+
+#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
+   (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
+   (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
+#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
+ (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
+ (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
+static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
+{
+CPUState *cs = CPU(dev);
+X86CPU *cpu = X86_CPU(dev);
+X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
+CPUX86State *env = >env;
+Error *local_err = NULL;
+static bool ht_warned;
+
+if (xcc->kvm_required && !kvm_enabled()) {
+char *name = x86_cpu_class_get_model_name(xcc);
+error_setg(_err, "CPU model '%s' requires KVM", name);
+g_free(name);
+goto out;
+}
+
+if (cpu->apic_id == UNASSIGNED_APIC_ID) {
+error_setg(errp, "apic-id property was not initialized properly");
+return;
+}
+
+x86_cpu_load_features(cpu, _err);
+if (local_err) {
+goto out;
+}
+
 if (cpu->check_cpuid || cpu->enforce_cpuid) {
 if (x86_cpu_report_filtered_features(cpu) && cpu->enforce_cpuid) {
 error_setg(_err,
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 01/11] tests: Add test case for x86 feature parsing compatibility

[Jonathan Neuschäfer]

On Thu, Sep 29, 2016 at 06:14:49PM -0300, Eduardo Habkost wrote:
> Add a new test case to ensure the existing behavior of the
> feature parsing code wlil be kept.

s/wlil/will/

> 
> Signed-off-by: Eduardo Habkost 


Jonathan Neuschäfer


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v4 07/11] target-i386: xsave: Add FP and SSE bits to x86_ext_save_areas

[Eduardo Habkost]

Instead of treating the FP and SSE bits as special cases, add
them to the x86_ext_save_areas array. This will simplify the code
that calculates the supported xsave components and the size of
the xsave area.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c013ed0..b36388e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -535,6 +535,20 @@ typedef struct ExtSaveArea {
 } ExtSaveArea;
 
 static const ExtSaveArea x86_ext_save_areas[] = {
+[XSTATE_FP_BIT] = {
+/* x87 FP state component is always enabled if XSAVE is supported */
+.feature = FEAT_1_ECX, .bits = CPUID_EXT_XSAVE,
+/* x87 state is in the legacy region of the XSAVE area */
+.offset = 0,
+.size = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader),
+},
+[XSTATE_SSE_BIT] = {
+/* SSE state component is always enabled if XSAVE is supported */
+.feature = FEAT_1_ECX, .bits = CPUID_EXT_XSAVE,
+/* SSE state is in the legacy region of the XSAVE area */
+.offset = 0,
+.size = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader),
+},
 [XSTATE_YMM_BIT] =
   { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX,
 .offset = offsetof(X86XSaveArea, avx_state),
@@ -568,9 +582,9 @@ static const ExtSaveArea x86_ext_save_areas[] = {
 static uint32_t xsave_area_size(uint64_t mask)
 {
 int i;
-uint64_t ret = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader);
+uint64_t ret = 0;
 
-for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) {
+for (i = 0; i < ARRAY_SIZE(x86_ext_save_areas); i++) {
 const ExtSaveArea *esa = _ext_save_areas[i];
 if ((mask >> i) & 1) {
 ret = MAX(ret, esa->offset + esa->size);
@@ -2963,8 +2977,8 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
 return;
 }
 
-mask = (XSTATE_FP_MASK | XSTATE_SSE_MASK);
-for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) {
+mask = 0;
+for (i = 0; i < ARRAY_SIZE(x86_ext_save_areas); i++) {
 const ExtSaveArea *esa = _ext_save_areas[i];
 if (env->features[esa->feature] & esa->bits) {
 mask |= (1ULL << i);
-- 
2.7.4

[Qemu-devel] [PATCH v4 04/11] target-i386: Make plus_features/minus_features QOM-based

[Eduardo Habkost]

Instead of using custom feature name lookup code for
plus_features/minus_features, save the property names used in
"[+-]feature" and use object_property_set_bool() to set them.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 108 +++---
 1 file changed, 22 insertions(+), 86 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3d3f64e..4eaec0e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -650,85 +650,6 @@ void host_cpuid(uint32_t function, uint32_t count,
 *edx = vec[3];
 }
 
-#define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
-
-/* general substring compare of *[s1..e1) and *[s2..e2).  sx is start of
- * a substring.  ex if !NULL points to the first char after a substring,
- * otherwise the string is assumed to sized by a terminating nul.
- * Return lexical ordering of *s1:*s2.
- */
-static int sstrcmp(const char *s1, const char *e1,
-   const char *s2, const char *e2)
-{
-for (;;) {
-if (!*s1 || !*s2 || *s1 != *s2)
-return (*s1 - *s2);
-++s1, ++s2;
-if (s1 == e1 && s2 == e2)
-return (0);
-else if (s1 == e1)
-return (*s2);
-else if (s2 == e2)
-return (*s1);
-}
-}
-
-/* compare *[s..e) to *altstr.  *altstr may be a simple string or multiple
- * '|' delimited (possibly empty) strings in which case search for a match
- * within the alternatives proceeds left to right.  Return 0 for success,
- * non-zero otherwise.
- */
-static int altcmp(const char *s, const char *e, const char *altstr)
-{
-const char *p, *q;
-
-for (q = p = altstr; ; ) {
-while (*p && *p != '|')
-++p;
-if ((q == p && !*s) || (q != p && !sstrcmp(s, e, q, p)))
-return (0);
-if (!*p)
-return (1);
-else
-q = ++p;
-}
-}
-
-/* search featureset for flag *[s..e), if found set corresponding bit in
- * *pval and return true, otherwise return false
- */
-static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
-   const char **featureset)
-{
-uint32_t mask;
-const char **ppc;
-bool found = false;
-
-for (mask = 1, ppc = featureset; mask; mask <<= 1, ++ppc) {
-if (*ppc && !altcmp(s, e, *ppc)) {
-*pval |= mask;
-found = true;
-}
-}
-return found;
-}
-
-static void add_flagname_to_bitmaps(const char *flagname,
-FeatureWordArray words,
-Error **errp)
-{
-FeatureWord w;
-for (w = 0; w < FEATURE_WORDS; w++) {
-FeatureWordInfo *wi = _word_info[w];
-if (lookup_feature([w], flagname, NULL, wi->feat_names)) {
-break;
-}
-}
-if (w == FEATURE_WORDS) {
-error_setg(errp, "CPU feature %s not found", flagname);
-}
-}
-
 /* CPU class name definitions: */
 
 #define X86_CPU_TYPE_SUFFIX "-" TYPE_X86_CPU
@@ -2015,8 +1936,7 @@ static inline void feat2prop(char *s)
  * feat=on|feat even if the later is parsed after +-feat
  * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
  */
-static FeatureWordArray plus_features = { 0 };
-static FeatureWordArray minus_features = { 0 };
+static GList *plus_features, *minus_features;
 
 /* Parse "+feature,-feature,feature=foo" CPU feature string
  */
@@ -2047,10 +1967,14 @@ static void x86_cpu_parse_featurestr(const char 
*typename, char *features,
 
 /* Compatibility syntax: */
 if (featurestr[0] == '+') {
-add_flagname_to_bitmaps(featurestr + 1, plus_features, _err);
+feat2prop(featurestr + 1);
+plus_features = g_list_append(plus_features,
+  g_strdup(featurestr + 1));
 continue;
 } else if (featurestr[0] == '-') {
-add_flagname_to_bitmaps(featurestr + 1, minus_features, 
_err);
+feat2prop(featurestr + 1);
+minus_features = g_list_append(minus_features,
+   g_strdup(featurestr + 1));
 continue;
 }
 
@@ -3066,6 +2990,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 Error *local_err = NULL;
 static bool ht_warned;
 FeatureWord w;
+GList *l;
 
 if (xcc->kvm_required && !kvm_enabled()) {
 char *name = x86_cpu_class_get_model_name(xcc);
@@ -3091,9 +3016,20 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 }
 }
 
-for (w = 0; w < FEATURE_WORDS; w++) {
-cpu->env.features[w] |= plus_features[w];
-cpu->env.features[w] &= ~minus_features[w];
+for (l = plus_features; l; l = l->next) {
+const char *prop = l->data;
+object_property_set_bool(OBJECT(cpu), true, prop, _err);
+if (local_err) {
+

Re: [Qemu-devel] [PATCH 1/2] vhost: enable any layout feature

[Maxime Coquelin]

On 09/29/2016 10:21 PM, Michael S. Tsirkin wrote:

On Thu, Sep 29, 2016 at 10:05:22PM +0200, Maxime Coquelin wrote:



On 09/29/2016 07:57 PM, Michael S. Tsirkin wrote:

On Thu, Sep 29, 2016 at 05:30:53PM +0200, Maxime Coquelin wrote:

...


Before enabling anything by default, we should first optimize the 1 slot
case. Indeed, micro-benchmark using testpmd in txonly[0] shows ~17%
perf regression for 64 bytes case:
 - 2 descs per packet: 11.6Mpps
 - 1 desc per packet: 9.6Mpps

This is due to the virtio header clearing in virtqueue_enqueue_xmit().
Removing it, we get better results than with 2 descs (1.20Mpps).
Since the Virtio PMD doesn't support offloads, I wonder whether we can
just drop the memset?


What will happen? Will the header be uninitialized?

Yes..
I didn't look closely at the spec, but just looked at DPDK's and Linux
vhost implementations. IIUC, the header is just skipped in the two
implementations.


In linux guest skbs are initialized AFAIK. See virtio_net_hdr_from_skb
first thing it does is
memset(hdr, 0, sizeof(*hdr));


I meant in vhost-net linux implementation, the header is just skipped:

static void handle_tx(struct vhost_net *net)
{
...
/* Skip header. TODO: support TSO. */
len = iov_length(vq->iov, out);
iov_iter_init(_iter, WRITE, vq->iov, out, len);
iov_iter_advance(_iter, hdr_size);

And the same is done is done in DPDK:

static inline int __attribute__((always_inline))
copy_desc_to_mbuf(struct virtio_net *dev, struct vring_desc *descs,
  uint16_t max_desc, struct rte_mbuf *m, uint16_t desc_idx,
  struct rte_mempool *mbuf_pool)
{
...
/*
 * A virtio driver normally uses at least 2 desc buffers
 * for Tx: the first for storing the header, and others
 * for storing the data.
 */
if (likely((desc->len == dev->vhost_hlen) &&
   (desc->flags & VRING_DESC_F_NEXT) != 0)) {
desc = [desc->next];
if (unlikely(desc->flags & VRING_DESC_F_INDIRECT))
return -1;

desc_addr = gpa_to_vva(dev, desc->addr);
if (unlikely(!desc_addr))
return -1;

rte_prefetch0((void *)(uintptr_t)desc_addr);

desc_offset = 0;
desc_avail  = desc->len;
nr_desc+= 1;

PRINT_PACKET(dev, (uintptr_t)desc_addr, desc->len, 0);
} else {
desc_avail  = desc->len - dev->vhost_hlen;
desc_offset = dev->vhost_hlen;
}






The spec says:
The driver can send a completely checksummed packet. In this case, flags
will be zero, and gso_type
will be VIRTIO_NET_HDR_GSO_NONE.

and
The driver MUST set num_buffers to zero.
If VIRTIO_NET_F_CSUM is not negotiated, the driver MUST set flags to
zero and SHOULD supply a fully
checksummed packet to the device.

and
If none of the VIRTIO_NET_F_HOST_TSO4, TSO6 or UFO options have been
negotiated, the driver MUST
set gso_type to VIRTIO_NET_HDR_GSO_NONE.

so doing this unconditionally would be a spec violation, but if you see
value in this, we can add a feature bit.

Right it would be a spec violation, so it should be done conditionally.
If a feature bit is to be added, what about VIRTIO_NET_F_NO_TX_HEADER?
It would imply VIRTIO_NET_F_CSUM not set, and no GSO features set.
If negotiated, we wouldn't need to prepend a header.


Yes but two points.

1. why is this memset expensive? Is the test completely skipping looking
   at the packet otherwise?

Yes.


2. As long as we are doing this, see
Alignment vs. Networking

in Documentation/unaligned-memory-access.txt

Thanks, I'll have a look tomorrow.

Maxime

[Qemu-devel] [PATCH v4 11/11] target-i386: Return runnability information on query-cpu-definitions

[Eduardo Habkost]

Fill the "unavailable-features" field on the x86 implementation
of query-cpu-definitions.

Cc: Jiri Denemark 
Cc: libvir-l...@redhat.com
Signed-off-by: Eduardo Habkost 
---
Changes v3 -> v4:
* Handle missing XSAVE components cleanly, but looking up
  the original feature that required it
* Use x86_cpu_load_features() function

Changes v2 -> v3:
* Create a x86_cpu_feature_name() function, to
  isolate the code that returns the property name

Changes v1 -> v2:
* Updated to the new schema: no @runnable field, and
  always report @unavailable-features as present
---
 target-i386/cpu.c | 73 +++
 1 file changed, 73 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e099892..4dd3aee 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1945,6 +1945,27 @@ static inline void feat2prop(char *s)
 }
 }
 
+/* Return the feature property name for a feature flag bit */
+static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
+{
+/* XSAVE components are automatically enabled by other features,
+ * so return the original feature name instead
+ */
+if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
+int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
+
+if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
+x86_ext_save_areas[comp].bits) {
+w = x86_ext_save_areas[comp].feature;
+bitnr = ctz32(x86_ext_save_areas[comp].bits);
+}
+}
+
+assert(bitnr < 32);
+assert(w < FEATURE_WORDS);
+return feature_word_info[w].feat_names[bitnr];
+}
+
 /* Compatibily hack to maintain legacy +-feat semantic,
  * where +-feat overwrites any feature set by
  * feat=on|feat even if the later is parsed after +-feat
@@ -2032,6 +2053,56 @@ static void x86_cpu_parse_featurestr(const char 
*typename, char *features,
 }
 }
 
+static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
+
+/* Check for missing features that may prevent the CPU class from
+ * running using the current machine and accelerator.
+ */
+static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
+ strList **missing_feats)
+{
+X86CPU *xc;
+FeatureWord w;
+Error *err = NULL;
+strList **next = missing_feats;
+
+if (xcc->kvm_required && !kvm_enabled()) {
+strList *new = g_new0(strList, 1);
+new->value = g_strdup("kvm");;
+*missing_feats = new;
+return;
+}
+
+xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
+
+x86_cpu_load_features(xc, );
+if (err) {
+/* Errors at x86_cpu_load_features should never happen,
+ * but in case it does, just report the model as not
+ * runnable at all using the "type" property.
+ */
+strList *new = g_new0(strList, 1);
+new->value = g_strdup("type");
+*next = new;
+next = >next;
+}
+
+for (w = 0; w < FEATURE_WORDS; w++) {
+uint32_t filtered = xc->filtered_features[w];
+int i;
+for (i = 0; i < 32; i++) {
+if (filtered & (1UL << i)) {
+strList *new = g_new0(strList, 1);
+new->value = g_strdup(x86_cpu_feature_name(w, i));
+*next = new;
+next = >next;
+}
+}
+}
+
+object_unref(OBJECT(xc));
+}
+
 /* Print all cpuid feature names in featureset
  */
 static void listflags(FILE *f, fprintf_function print, const char **featureset)
@@ -2124,6 +2195,8 @@ static void x86_cpu_definition_entry(gpointer data, 
gpointer user_data)
 
 info = g_malloc0(sizeof(*info));
 info->name = x86_cpu_class_get_model_name(cc);
+x86_cpu_class_check_missing_features(cc, >unavailable_features);
+info->has_unavailable_features = true;
 
 entry = g_malloc0(sizeof(*entry));
 entry->value = info;
-- 
2.7.4

[Qemu-devel] [PATCH v4 10/11] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

Add a new optional field to query-cpu-definitions schema:
"unavailable-features". It will contain a list of QOM properties
that prevent the CPU model from running in the current host.

Cc: David Hildenbrand 
Cc: Michael Mueller 
Cc: Christian Borntraeger 
Cc: Cornelia Huck 
Cc: Jiri Denemark 
Cc: libvir-l...@redhat.com
Reviewed-by: Eric Blake 
Signed-off-by: Eduardo Habkost 
---
Changes v3 -> v4:
* Changed doc to "since 2.8" as we missed 2.7
  * Reported-by: Eric Blake 

Changes v2 -> v3:
* Small documentation reword
  * Suggested-by: Markus Armbruster 

Changes v1 -> v2:
* Remove @runnable field, non-empty @unavailable-features is
  enough to report CPU model as not runnable.
* Documentation updates:
  * Changed to "(since 2.7)";
  * Add more details about the exact meaning of
unavailable-features, and what it would mean to see
read-only QOM properties in the list, and that
implementations can return "type" if there's
no extra information available;
---
 qapi-schema.json | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index c3dcf11..abb163b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3114,10 +3114,31 @@
 #  QEMU version, machine type, machine options and accelerator options.
 #  A static model is always migration-safe. (since 2.8)
 #
+# @unavailable-features: #optional List of properties that prevent
+#the CPU model from running in the current
+#host. (since 2.8)
+#
+# @unavailable-features is a list of QOM property names that
+# represent CPU model attributes that prevent the CPU from running.
+# If the QOM property is read-only, that means there's no known
+# way to make the CPU model run in the current host. Implementations
+# that choose not to provide specific information return the
+# property name "type".
+# If the property is read-write, it means that it MAY be possible
+# to run the CPU model in the current host if that property is
+# changed. Management software can use it as hints to suggest or
+# choose an alternative for the user, or just to generate meaningful
+# error messages explaining why the CPU model can't be used.
+# If @unavailable-features is an empty list, the CPU model is
+# runnable using the current host and machine-type.
+# If @unavailable-features is not present, runnability
+# information for the CPU is not available.
+#
 # Since: 1.2.0
 ##
 { 'struct': 'CpuDefinitionInfo',
-  'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool' } }
+  'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
+'*unavailable-features': [ 'str' ] } }
 
 ##
 # @query-cpu-definitions:
-- 
2.7.4

[Qemu-devel] [PATCH v4 03/11] target-i386: Disable VME by default with TCG

[Eduardo Habkost]

VME is already disabled automatically when using TCG. So, instead
of pretending it is there when reporting CPU model data on
query-cpu-* QMP commands (making every CPU model to be reported
as not runnable), we can disable it by default on all CPU models
when using TCG.

Do that by adding a tcg_default_props array that will work like
kvm_default_props.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ac3646e..3d3f64e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1550,6 +1550,14 @@ static PropValue kvm_default_props[] = {
 { NULL, NULL },
 };
 
+/* TCG-specific defaults that override all CPU models when using TCG
+ */
+static PropValue tcg_default_props[] = {
+{ "vme", "off" },
+{ NULL, NULL },
+};
+
+
 void x86_cpu_change_kvm_default(const char *prop, const char *value)
 {
 PropValue *pv;
@@ -2283,6 +2291,8 @@ static void x86_cpu_load_def(X86CPU *cpu, 
X86CPUDefinition *def, Error **errp)
 }
 
 x86_cpu_apply_props(cpu, kvm_default_props);
+} else if (tcg_enabled()) {
+x86_cpu_apply_props(cpu, tcg_default_props);
 }
 
 env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
-- 
2.7.4

[Qemu-devel] [PATCH v4 08/11] target-i386: Move warning code outside x86_cpu_filter_features()

[Eduardo Habkost]

x86_cpu_filter_features() will be reused by code that shouldn't
print any warning. Move the warning code to a new
x86_cpu_report_filtered_features() function, and call it from
x86_cpu_realizefn().

Signed-off-by: Eduardo Habkost 
---
Changes v3 -> v4:
* Made x86_cpu_filter_features() void, make
  x86_cpu_report_filtered_features() return true if
  some features were filtered
---
 target-i386/cpu.c | 41 -
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b36388e..b25657b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2163,14 +2163,11 @@ static uint32_t 
x86_cpu_get_supported_feature_word(FeatureWord w,
 
 /*
  * Filters CPU feature words based on host availability of each feature.
- *
- * Returns: 0 if all flags are supported by the host, non-zero otherwise.
  */
-static int x86_cpu_filter_features(X86CPU *cpu)
+static void x86_cpu_filter_features(X86CPU *cpu)
 {
 CPUX86State *env = >env;
 FeatureWord w;
-int rv = 0;
 
 for (w = 0; w < FEATURE_WORDS; w++) {
 uint32_t host_feat =
@@ -2178,15 +2175,22 @@ static int x86_cpu_filter_features(X86CPU *cpu)
 uint32_t requested_features = env->features[w];
 env->features[w] &= host_feat;
 cpu->filtered_features[w] = requested_features & ~env->features[w];
-if (cpu->filtered_features[w]) {
-if (cpu->check_cpuid || cpu->enforce_cpuid) {
-report_unavailable_features(w, cpu->filtered_features[w]);
-}
-rv = 1;
-}
 }
+}
 
-return rv;
+/* Report list of filtered features to stderr.
+ * Returns true if some features were found to be filtered, false otherwise
+ */
+static bool x86_cpu_report_filtered_features(X86CPU *cpu)
+{
+FeatureWord w;
+uint32_t filtered = 0;
+
+for (w = 0; w < FEATURE_WORDS; w++) {
+filtered |= cpu->filtered_features[w];
+report_unavailable_features(w, cpu->filtered_features[w]);
+}
+return filtered;
 }
 
 static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
@@ -3082,12 +3086,15 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
 }
 
-if (x86_cpu_filter_features(cpu) && cpu->enforce_cpuid) {
-error_setg(_err,
-   kvm_enabled() ?
-   "Host doesn't support requested features" :
-   "TCG doesn't support requested features");
-goto out;
+x86_cpu_filter_features(cpu);
+if (cpu->check_cpuid || cpu->enforce_cpuid) {
+if (x86_cpu_report_filtered_features(cpu) && cpu->enforce_cpuid) {
+error_setg(_err,
+   kvm_enabled() ?
+   "Host doesn't support requested features" :
+   "TCG doesn't support requested features");
+goto out;
+}
 }
 
 /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
-- 
2.7.4

[Qemu-devel] [PATCH v4 06/11] target-i386: Register properties for feature aliases manually

[Eduardo Habkost]

Instead of keeping the aliases inside the feature name arrays and
require parsing the strings, just register alias properties
manually. This simplifies the property registration code and will
simplify code that needs to look up property names for CPUID
bits.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 44 ++--
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7795a7c..c013ed0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -278,12 +278,12 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = 
{
 },
 [FEAT_1_ECX] = {
 .feat_names = {
-"pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", 
"monitor",
+"pni" /* Intel,AMD sse3 */, "pclmulqdq", "dtes64", "monitor",
 "ds-cpl", "vmx", "smx", "est",
 "tm2", "ssse3", "cid", NULL,
 "fma", "cx16", "xtpr", "pdcm",
-NULL, "pcid", "dca", "sse4.1|sse4-1",
-"sse4.2|sse4-2", "x2apic", "movbe", "popcnt",
+NULL, "pcid", "dca", "sse4.1",
+"sse4.2", "x2apic", "movbe", "popcnt",
 "tsc-deadline", "aes", "xsave", "osxsave",
 "avx", "f16c", "rdrand", "hypervisor",
 },
@@ -302,9 +302,9 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 NULL /* cx8 */, NULL /* apic */, NULL, "syscall",
 NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
 NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
-"nx|xd", NULL, "mmxext", NULL /* mmx */,
-NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb", "rdtscp",
-NULL, "lm|i64", "3dnowext", "3dnow",
+"nx", NULL, "mmxext", NULL /* mmx */,
+NULL /* fxsr */, "fxsr-opt", "pdpe1gb", "rdtscp",
+NULL, "lm", "3dnowext", "3dnow",
 },
 .cpuid_eax = 0x8001, .cpuid_reg = R_EDX,
 .tcg_features = TCG_EXT2_FEATURES,
@@ -3323,28 +3323,19 @@ static void x86_cpu_register_feature_bit_props(X86CPU 
*cpu,
FeatureWord w,
int bitnr)
 {
-Object *obj = OBJECT(cpu);
-int i;
-char **names;
 FeatureWordInfo *fi = _word_info[w];
+const char *name = fi->feat_names[bitnr];
 
-if (!fi->feat_names[bitnr]) {
+if (!name) {
 return;
 }
 
-names = g_strsplit(fi->feat_names[bitnr], "|", 0);
-
 /* Property names should use "-" instead of "_" */
-assert(!strchr(names[0], '_'));
-x86_cpu_register_bit_prop(cpu, names[0], >env.features[w], bitnr);
-
-for (i = 1; names[i]; i++) {
-feat2prop(names[i]);
-object_property_add_alias(obj, names[i], obj, names[0],
-  _abort);
-}
-
-g_strfreev(names);
+assert(!strchr(name, '_'));
+/* aliases don't use "|" delimiters anymore, they are registered
+ * manually using object_property_add_alias() */
+assert(!strchr(name, '|'));
+x86_cpu_register_bit_prop(cpu, name, >env.features[w], bitnr);
 }
 
 static void x86_cpu_initfn(Object *obj)
@@ -3392,6 +3383,15 @@ static void x86_cpu_initfn(Object *obj)
 }
 }
 
+/* Alias for feature properties: */
+object_property_add_alias(obj, "sse3", obj, "pni", _abort);
+object_property_add_alias(obj, "pclmuldq", obj, "pclmulqdq", _abort);
+object_property_add_alias(obj, "sse4-1", obj, "sse4.1", _abort);
+object_property_add_alias(obj, "sse4-2", obj, "sse4.2", _abort);
+object_property_add_alias(obj, "xd", obj, "nx", _abort);
+object_property_add_alias(obj, "ffxsr", obj, "fxsr-opt", _abort);
+object_property_add_alias(obj, "i64", obj, "lm", _abort);
+
 x86_cpu_load_def(cpu, xcc->cpu_def, _abort);
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v4 00/11] Add runnability info to query-cpu-definitions

[Eduardo Habkost]

This series extends query-cpu-definitions to include an extra
field: "unavailable-features". The new field can be used to find
out reasons that prevent the CPU model from running in the
current host.

This will return information based on the current machine and
accelerator only. In the future we may extend these mechanisms to
allow querying other machines and other accelerators without
restarting QEMU, but it will require some reorganization of
QEMU's main code.

To be able to implement this more cleanly, the series rework some
of the feature/property name code.

This series can be seen in the git branch at:
  https://github.com/ehabkost/qemu-hacks.git 
work/query-cpu-definitions-runnable-info

The series is based on my x86-next branch:
   https://github.com/ehabkost/qemu.git x86-next

Changes v3 -> v4:
* Removed patch "Define CPUID filtering functions before x86_cpu_list"
* New patch: "tests: Add test case for x86 feature parsing compatibility"
* New patch: "target-i386: Disable VME by default with TCG"
  * Disable VME by default on TCG to avoid returning bogus
results for all CPU models in TCG mode
* New patch: "target-i386: Make plus_features/minus_features QOM-based"
* New patch: "target-i386: Remove underscores from property names"
* New patch: "target-i386: Register properties for feature aliases manually"
* New patch: "target-i386: xsave: Add FP and SSE bits to x86_ext_save_areas"
* New patch: "target-i386: x86_cpu_load_features() function"
* On patch: "target-i386: Return runnability information on 
query-cpu-definitions"
  * Added code to handle unsupported XSAVE components cleanly
  * Use x86_cpu_load_features() function

Changes v2 -> v3:
* Small documentation reword
  * Suggested-by: Markus Armbruster 
* Create a x86_cpu_feature_name() function, to
  isolate the code that returns the property name

Changes v1 -> v2:
* Fixed documentation to say "(since 2.7)"
* Removed @runnable field, improved documentation

Example command output:

{ "return": [
{
"unavailable-features": [],
"static": false,
"name": "host"
},
{
"unavailable-features": [],
"static": false,
"name": "qemu64"
},
{
"unavailable-features": [],
"static": false,
"name": "qemu32"
},
{
"unavailable-features": ["npt", "sse4a", "3dnow", "3dnowext", 
"fxsr-opt", "mmxext"],
"static": false,
"name": "phenom"
},
{
"unavailable-features": [],
"static": false,
"name": "pentium3"
},
{
"unavailable-features": [],
"static": false,
"name": "pentium2"
},
{
"unavailable-features": [],
"static": false,
"name": "pentium"
},
{
"unavailable-features": [],
"static": false,
"name": "n270"
},
{
"unavailable-features": [],
"static": false,
"name": "kvm64"
},
{
"unavailable-features": [],
"static": false,
"name": "kvm32"
},
{
"unavailable-features": [],
"static": false,
"name": "coreduo"
},
{
"unavailable-features": [],
"static": false,
"name": "core2duo"
},
{
"unavailable-features": ["3dnow", "3dnowext", "mmxext"],
"static": false,
"name": "athlon"
},
{
"unavailable-features": [],
"static": false,
"name": "Westmere"
},
{
"unavailable-features": ["xgetbv1", "xsavec", "3dnowprefetch", 
"smap", "adx", "rdseed", "mpx", "rtm", "hle"],
"static": false,
"name": "Skylake-Client"
},
{
"unavailable-features": [],
"static": false,
"name": "SandyBridge"
},
{
"unavailable-features": [],
"static": false,
"name": "Penryn"
},
{
"unavailable-features": ["tbm", "fma4", "xop", "3dnowprefetch", 
"misalignsse", "sse4a"],
"static": false,
"name": "Opteron_G5"
},
{
"unavailable-features": ["fma4", "xop", "3dnowprefetch", 
"misalignsse", "sse4a"],
"static": false,
"name": "Opteron_G4"
},
{
"unavailable-features": ["misalignsse", "sse4a"],
"static": false,
"name": "Opteron_G3"
},
{
"unavailable-features": [],
"static": false,
"name": "Opteron_G2"
},
{
"unavailable-features": [],
"static": false,
"name": "Opteron_G1"
},
{

[Qemu-devel] [PATCH v4 02/11] target-i386: List CPU models using subclass list

[Eduardo Habkost]

Instead of using the builtin_x86_defs array, use the QOM subclass
list to list CPU models on "-cpu ?" and "query-cpu-definitions".

Signed-off-by: Andreas Färber 
[ehabkost: copied code from a patch by Andreas:
 "target-i386: QOM'ify CPU", from March 2012]
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu-qom.h |   4 ++
 target-i386/cpu.c | 103 --
 2 files changed, 78 insertions(+), 29 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 5dde658..e724004 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -63,6 +63,10 @@ typedef struct X86CPUClass {
 
 bool kvm_required;
 
+/* Optional description of CPU model.
+ * If unavailable, cpu_def->model_id is used */
+const char *model_description;
+
 DeviceRealize parent_realize;
 void (*parent_reset)(CPUState *cpu);
 } X86CPUClass;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0807e92..ac3646e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1628,6 +1628,9 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void 
*data)
 cpu_x86_fill_model_id(host_cpudef.model_id);
 
 xcc->cpu_def = _cpudef;
+xcc->model_description =
+"KVM processor with all supported host features "
+"(only available in KVM mode)";
 
 /* level, xlevel, xlevel2, and the feature words are initialized on
  * instance_init, because they require KVM to be initialized.
@@ -2098,23 +2101,62 @@ static void listflags(FILE *f, fprintf_function print, 
const char **featureset)
 }
 }
 
-/* generate CPU information. */
+/* Sort alphabetically by type name, listing kvm_required models last. */
+static gint x86_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+ObjectClass *class_a = (ObjectClass *)a;
+ObjectClass *class_b = (ObjectClass *)b;
+X86CPUClass *cc_a = X86_CPU_CLASS(class_a);
+X86CPUClass *cc_b = X86_CPU_CLASS(class_b);
+const char *name_a, *name_b;
+
+if (cc_a->kvm_required != cc_b->kvm_required) {
+/* kvm_required items go last */
+return cc_a->kvm_required ? 1 : -1;
+} else {
+name_a = object_class_get_name(class_a);
+name_b = object_class_get_name(class_b);
+return strcmp(name_a, name_b);
+}
+}
+
+static GSList *get_sorted_cpu_model_list(void)
+{
+GSList *list = object_class_get_list(TYPE_X86_CPU, false);
+list = g_slist_sort(list, x86_cpu_list_compare);
+return list;
+}
+
+static void x86_cpu_list_entry(gpointer data, gpointer user_data)
+{
+ObjectClass *oc = data;
+X86CPUClass *cc = X86_CPU_CLASS(oc);
+CPUListState *s = user_data;
+char *name = x86_cpu_class_get_model_name(cc);
+const char *desc = cc->model_description;
+if (!desc) {
+desc = cc->cpu_def->model_id;
+}
+
+(*s->cpu_fprintf)(s->file, "x86 %16s  %-48s\n",
+  name, desc);
+g_free(name);
+}
+
+/* list available CPU models and flags */
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
-X86CPUDefinition *def;
-char buf[256];
 int i;
+CPUListState s = {
+.file = f,
+.cpu_fprintf = cpu_fprintf,
+};
+GSList *list;
 
-for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-def = _x86_defs[i];
-snprintf(buf, sizeof(buf), "%s", def->name);
-(*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
-}
-#ifdef CONFIG_KVM
-(*cpu_fprintf)(f, "x86 %16s  %-48s\n", "host",
-   "KVM processor with all supported host features "
-   "(only available in KVM mode)");
-#endif
+(*cpu_fprintf)(f, "Available CPUs:\n");
+list = get_sorted_cpu_model_list();
+g_slist_foreach(list, x86_cpu_list_entry, );
+g_slist_free(list);
 
 (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
 for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
@@ -2126,26 +2168,29 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 }
 }
 
-CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
+static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
 {
-CpuDefinitionInfoList *cpu_list = NULL;
-X86CPUDefinition *def;
-int i;
+ObjectClass *oc = data;
+X86CPUClass *cc = X86_CPU_CLASS(oc);
+CpuDefinitionInfoList **cpu_list = user_data;
+CpuDefinitionInfoList *entry;
+CpuDefinitionInfo *info;
 
-for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-CpuDefinitionInfoList *entry;
-CpuDefinitionInfo *info;
-
-def = _x86_defs[i];
-info = g_malloc0(sizeof(*info));
-info->name = g_strdup(def->name);
+info = g_malloc0(sizeof(*info));
+info->name = x86_cpu_class_get_model_name(cc);
 
-entry = g_malloc0(sizeof(*entry));
-entry->value = info;
-entry->next = cpu_list;
-cpu_list = entry;
-}
+entry = g_malloc0(sizeof(*entry));
+

[Qemu-devel] [PATCH v4 01/11] tests: Add test case for x86 feature parsing compatibility

[Eduardo Habkost]

Add a new test case to ensure the existing behavior of the
feature parsing code wlil be kept.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 tests/test-x86-cpuid-compat.c | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/tests/test-x86-cpuid-compat.c b/tests/test-x86-cpuid-compat.c
index 83162a4..7cff2b5 100644
--- a/tests/test-x86-cpuid-compat.c
+++ b/tests/test-x86-cpuid-compat.c
@@ -3,6 +3,7 @@
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qint.h"
+#include "qapi/qmp/qbool.h"
 #include "libqtest.h"
 
 static char *get_cpu0_qom_path(void)
@@ -34,6 +35,15 @@ static QObject *qom_get(const char *path, const char *prop)
 return ret;
 }
 
+static bool qom_get_bool(const char *path, const char *prop)
+{
+QBool *value = qobject_to_qbool(qom_get(path, prop));
+bool b = qbool_get_bool(value);
+
+QDECREF(value);
+return b;
+}
+
 typedef struct CpuidTestArgs {
 const char *cmdline;
 const char *property;
@@ -66,10 +76,39 @@ static void add_cpuid_test(const char *name, const char 
*cmdline,
 qtest_add_data_func(name, args, test_cpuid_prop);
 }
 
+static void test_plus_minus(void)
+{
+char *path;
+
+/* Rules:
+ * "-foo" overrides "+foo"
+ * "[+-]foo" overrides "foo=..."
+ * "foo_bar" should be translated to "foo-bar"
+ */
+qtest_start("-cpu 
pentium,-fpu,+fpu,-mce,mce=on,+cx8,cx8=off,+sse4_1,sse4_2=on");
+path = get_cpu0_qom_path();
+
+g_assert_false(qom_get_bool(path, "fpu"));
+g_assert_false(qom_get_bool(path, "mce"));
+g_assert_true(qom_get_bool(path, "cx8"));
+
+/* Test both the original and the alias feature names: */
+g_assert_true(qom_get_bool(path, "sse4-1"));
+g_assert_true(qom_get_bool(path, "sse4.1"));
+
+g_assert_true(qom_get_bool(path, "sse4-2"));
+g_assert_true(qom_get_bool(path, "sse4.2"));
+
+qtest_end();
+g_free(path);
+}
+
 int main(int argc, char **argv)
 {
 g_test_init(, , NULL);
 
+qtest_add_func("x86/cpuid/parsing-plus-minus", test_plus_minus);
+
 /* Original level values for CPU models: */
 add_cpuid_test("x86/cpuid/phenom/level",
"-cpu phenom", "level", 5);
-- 
2.7.4

[Qemu-devel] [PATCH v4 05/11] target-i386: Remove underscores from property names

[Eduardo Habkost]

Instead of translating the feature name entries when adding
property names, store the actual property names in the feature
name array.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 31 ---
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4eaec0e..7795a7c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -279,11 +279,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = 
{
 [FEAT_1_ECX] = {
 .feat_names = {
 "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", 
"monitor",
-"ds_cpl", "vmx", "smx", "est",
+"ds-cpl", "vmx", "smx", "est",
 "tm2", "ssse3", "cid", NULL,
 "fma", "cx16", "xtpr", "pdcm",
-NULL, "pcid", "dca", "sse4.1|sse4_1",
-"sse4.2|sse4_2", "x2apic", "movbe", "popcnt",
+NULL, "pcid", "dca", "sse4.1|sse4-1",
+"sse4.2|sse4-2", "x2apic", "movbe", "popcnt",
 "tsc-deadline", "aes", "xsave", "osxsave",
 "avx", "f16c", "rdrand", "hypervisor",
 },
@@ -303,7 +303,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
 NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
 "nx|xd", NULL, "mmxext", NULL /* mmx */,
-NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb", "rdtscp",
+NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb", "rdtscp",
 NULL, "lm|i64", "3dnowext", "3dnow",
 },
 .cpuid_eax = 0x8001, .cpuid_reg = R_EDX,
@@ -311,13 +311,13 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = 
{
 },
 [FEAT_8000_0001_ECX] = {
 .feat_names = {
-"lahf_lm", "cmp_legacy", "svm", "extapic",
+"lahf-lm", "cmp-legacy", "svm", "extapic",
 "cr8legacy", "abm", "sse4a", "misalignsse",
 "3dnowprefetch", "osvw", "ibs", "xop",
 "skinit", "wdt", NULL, "lwp",
-"fma4", "tce", NULL, "nodeid_msr",
-NULL, "tbm", "topoext", "perfctr_core",
-"perfctr_nb", NULL, NULL, NULL,
+"fma4", "tce", NULL, "nodeid-msr",
+NULL, "tbm", "topoext", "perfctr-core",
+"perfctr-nb", NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 },
 .cpuid_eax = 0x8001, .cpuid_reg = R_ECX,
@@ -339,8 +339,8 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 },
 [FEAT_KVM] = {
 .feat_names = {
-"kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
-"kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt",
+"kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
+"kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
@@ -400,9 +400,9 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 },
 [FEAT_SVM] = {
 .feat_names = {
-"npt", "lbrv", "svm_lock", "nrip_save",
-"tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
-NULL, NULL, "pause_filter", NULL,
+"npt", "lbrv", "svm-lock", "nrip-save",
+"tsc-scale", "vmcb-clean",  "flushbyasid", "decodeassists",
+NULL, NULL, "pause-filter", NULL,
 "pfthreshold", NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
@@ -414,7 +414,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 },
 [FEAT_7_0_EBX] = {
 .feat_names = {
-"fsgsbase", "tsc_adjust", NULL, "bmi1",
+"fsgsbase", "tsc-adjust", NULL, "bmi1",
 "hle", "avx2", NULL, "smep",
 "bmi2", "erms", "invpcid", "rtm",
 NULL, NULL, "mpx", NULL,
@@ -3334,7 +3334,8 @@ static void x86_cpu_register_feature_bit_props(X86CPU 
*cpu,
 
 names = g_strsplit(fi->feat_names[bitnr], "|", 0);
 
-feat2prop(names[0]);
+/* Property names should use "-" instead of "_" */
+assert(!strchr(names[0], '_'));
 x86_cpu_register_bit_prop(cpu, names[0], >env.features[w], bitnr);
 
 for (i = 1; names[i]; i++) {
-- 
2.7.4

Re: [Qemu-devel] [Qemu-block] [PATCH 0/5] blockjobs: Fix transactional race condition

[John Snow]

On 09/29/2016 07:33 AM, Kevin Wolf wrote:

Am 28.09.2016 um 14:16 hat Vladimir Sementsov-Ogievskiy geschrieben:

I think jobs will need to remain "one coroutine, one job" for now,
but there's no reason why drive-backup or blockdev-backup can't
just create multiple jobs each if that's what they need to do.
(The backup job object could, in theory, just have another job
pointer to a helper job if it really became necessary.)


What's the problem with a job spawning additional coroutines internally?
Jobs already do this all the time (AIO requests are just coroutines in
disguise.)



Mostly I was just pushing back against baking in multi-coroutines 
per-job as a default. If you want to add them in your own extensions, 
e.g. MultiJob : BlockJob {

  Coroutine *extra;
}

then I'm okay with that and realize we already do exactly that. Beyond 
that I think complicates the job layer a little more than necessary... 
but maybe I am being too pre-fearful.



A job is basically just the user interface for managing a background
process, so helper jobs that are managed internally rather than by the
user don't seem to make that much sense.

Kevin

[Qemu-devel] [PATCH] CODING_STYLE: Fix a typo ("Many a flamewar")

[Jonathan Neuschäfer]

Signed-off-by: Jonathan Neuschäfer 
---
 CODING_STYLE | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CODING_STYLE b/CODING_STYLE
index e7fde15..a03a994 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -9,7 +9,7 @@ patches before submitting.
 Of course, the most important aspect in any coding style is whitespace.
 Crusty old coders who have trouble spotting the glasses on their noses
 can tell the difference between a tab and eight spaces from a distance
-of approximately fifteen parsecs.  Many a flamewar have been fought and
+of approximately fifteen parsecs.  Many flamewars have been fought and
 lost on this issue.
 
 QEMU indents are four spaces.  Tabs are never used, except in Makefiles
-- 
2.9.3

[Qemu-devel] [PULL 2/4] ahci: clear aiocb in ncq_cb

[John Snow]

Similar to existing fixes for IDE (87ac25fd) and ATAPI (7f951b2d), the
AIOCB must be cleared in the callback. Otherwise, we may accidentally
try to reset a dangling pointer in bdrv_aio_cancel() from a port reset.

Signed-off-by: John Snow 
Reviewed-by: Stefan Hajnoczi 
Message-id: 1474575040-32079-2-git-send-email-js...@redhat.com
Signed-off-by: John Snow 
---
 hw/ide/ahci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index f3438ad..63ead21 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -948,6 +948,7 @@ static void ncq_cb(void *opaque, int ret)
 NCQTransferState *ncq_tfs = (NCQTransferState *)opaque;
 IDEState *ide_state = _tfs->drive->port.ifs[0];
 
+ncq_tfs->aiocb = NULL;
 if (ret == -ECANCELED) {
 return;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] vhost: enable any layout feature

[Michael S. Tsirkin]

On Thu, Sep 29, 2016 at 10:05:22PM +0200, Maxime Coquelin wrote:
> 
> 
> On 09/29/2016 07:57 PM, Michael S. Tsirkin wrote:
> > On Thu, Sep 29, 2016 at 05:30:53PM +0200, Maxime Coquelin wrote:
> ...
> > > 
> > > Before enabling anything by default, we should first optimize the 1 slot
> > > case. Indeed, micro-benchmark using testpmd in txonly[0] shows ~17%
> > > perf regression for 64 bytes case:
> > >  - 2 descs per packet: 11.6Mpps
> > >  - 1 desc per packet: 9.6Mpps
> > > 
> > > This is due to the virtio header clearing in virtqueue_enqueue_xmit().
> > > Removing it, we get better results than with 2 descs (1.20Mpps).
> > > Since the Virtio PMD doesn't support offloads, I wonder whether we can
> > > just drop the memset?
> > 
> > What will happen? Will the header be uninitialized?
> Yes..
> I didn't look closely at the spec, but just looked at DPDK's and Linux
> vhost implementations. IIUC, the header is just skipped in the two
> implementations.

In linux guest skbs are initialized AFAIK. See virtio_net_hdr_from_skb
first thing it does is
memset(hdr, 0, sizeof(*hdr));



> > 
> > The spec says:
> > The driver can send a completely checksummed packet. In this case, flags
> > will be zero, and gso_type
> > will be VIRTIO_NET_HDR_GSO_NONE.
> > 
> > and
> > The driver MUST set num_buffers to zero.
> > If VIRTIO_NET_F_CSUM is not negotiated, the driver MUST set flags to
> > zero and SHOULD supply a fully
> > checksummed packet to the device.
> > 
> > and
> > If none of the VIRTIO_NET_F_HOST_TSO4, TSO6 or UFO options have been
> > negotiated, the driver MUST
> > set gso_type to VIRTIO_NET_HDR_GSO_NONE.
> > 
> > so doing this unconditionally would be a spec violation, but if you see
> > value in this, we can add a feature bit.
> Right it would be a spec violation, so it should be done conditionally.
> If a feature bit is to be added, what about VIRTIO_NET_F_NO_TX_HEADER?
> It would imply VIRTIO_NET_F_CSUM not set, and no GSO features set.
> If negotiated, we wouldn't need to prepend a header.

Yes but two points.

1. why is this memset expensive? Is the test completely skipping looking
   at the packet otherwise?

2. As long as we are doing this, see
Alignment vs. Networking

in Documentation/unaligned-memory-access.txt


> From the micro-benchmarks results, we can expect +10% compared to
> indirect descriptors, and + 5% compared to using 2 descs in the
> virtqueue.
> Also, it should have the same benefits as indirect descriptors for 0%
> pkt loss (as we can fill 2x more packets in the virtqueue).
> 
> What do you think?
> 
> Thanks,
> Maxime

[Qemu-devel] [PULL 3/4] MAINTAINERS: Add some more headers to the IDE section

[John Snow]

From: Thomas Huth 

The folder include/hw/ide/ belongs to the IDE section.

Signed-off-by: Thomas Huth 
Reviewed-by: John Snow 
Message-id: 1474646996-30421-1-git-send-email-th...@redhat.com
Signed-off-by: John Snow 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f3c1f7f..9b7e846 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -772,6 +772,7 @@ M: John Snow 
 L: qemu-bl...@nongnu.org
 S: Supported
 F: include/hw/ide.h
+F: include/hw/ide/
 F: hw/ide/
 F: hw/block/block.c
 F: hw/block/cdrom.c
-- 
2.7.4

[Qemu-devel] [PULL 0/4] IDE patches

[John Snow]

The following changes since commit c640f2849ee8775fe1bbd7a2772610aa77816f9f:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging 
(2016-09-28 23:02:56 +0100)

are available in the git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to ca44141d5fb801dd5903102acefd0f2d8e8bb6a1:

  ide: Fix memory leak in ide_register_restart_cb() (2016-09-29 15:50:29 -0400)





Ashijeet Acharya (1):
  ide: Fix memory leak in ide_register_restart_cb()

John Snow (2):
  ide: fix DMA register transitions
  ahci: clear aiocb in ncq_cb

Thomas Huth (1):
  MAINTAINERS: Add some more headers to the IDE section

 MAINTAINERS   |  1 +
 hw/ide/ahci.c |  1 +
 hw/ide/core.c |  4 ++--
 hw/ide/qdev.c | 11 +++
 include/hw/ide/internal.h |  1 +
 5 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.7.4

[Qemu-devel] [PULL 1/4] ide: fix DMA register transitions

[John Snow]

ATA8-APT defines the state transitions for both a host controller and
for the hardware device during the lifecycle of a DMA transfer, in
section 9.7 "DMA command protocol."

One of the interesting tidbits here is that when a device transitions
from DDMA0 ("Prepare state") to DDMA1 ("Data_Transfer State"), it can
choose to set either BSY or DRQ to signal this transition, but not both.

as ide_sector_dma_start is the last point in our preparation process
before we begin the real data transfer process (for either AHCI or BMDMA),
this is the correct transition point for DDMA0 to DDMA1.

I have chosen !BSY && DRQ for QEMU to make the transition from DDMA0 the
most obvious.

Reported-by: Benjamin David Lunt 
Signed-off-by: John Snow 
Reviewed-by: Kevin Wolf 
Tested-by: Stefan Weil 
Message-id: 1470175541-19344-1-git-send-email-js...@redhat.com
Signed-off-by: John Snow 
---
 hw/ide/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index b0e42a6..1bee18d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -908,7 +908,7 @@ eot:
 
 static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 {
-s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
+s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
 s->io_buffer_size = 0;
 s->dma_cmd = dma_cmd;
 
-- 
2.7.4