[Qemu-devel] Introduction

[Aishwarya Kadlag]

Hi, I am aish2k joining this mailing list today

Re: [Qemu-devel] [PATCH v6 1/3] pci: Add support for Designware IP block

[Andrey Smirnov]

On Sat, Mar 3, 2018 at 7:55 PM, Michael S. Tsirkin  wrote:
> On Tue, Feb 13, 2018 at 02:47:24PM -0800, Andrey Smirnov wrote:
>> On Tue, Feb 13, 2018 at 2:15 PM, Michael S. Tsirkin  wrote:
>> > On Tue, Feb 13, 2018 at 12:24:40PM -0800, Andrey Smirnov wrote:
>> >> On Tue, Feb 13, 2018 at 10:13 AM, Michael S. Tsirkin  
>> >> wrote:
>> >> > On Tue, Feb 13, 2018 at 09:07:10AM -0800, Andrey Smirnov wrote:
>> >> >> +static void designware_pcie_root_class_init(ObjectClass *klass, void 
>> >> >> *data)
>> >> >> +{
>> >> >> +PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> >> >> +DeviceClass *dc = DEVICE_CLASS(klass);
>> >> >> +
>> >> >> +set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>> >> >> +
>> >> >> +k->vendor_id = PCI_VENDOR_ID_SYNOPSYS;
>> >> >> +k->device_id = 0xABCD;
>> >> >> +k->revision = 0;
>> >> >> +k->class_id = PCI_CLASS_BRIDGE_PCI;
>> >> >> +k->is_express = true;
>> >> >> +k->is_bridge = true;
>> >> >> +k->exit = pci_bridge_exitfn;
>> >> >> +k->realize = designware_pcie_root_realize;
>> >> >> +k->config_read = designware_pcie_root_config_read;
>> >> >> +k->config_write = designware_pcie_root_config_write;
>> >> >> +
>> >> >> +dc->reset = pci_bridge_reset;
>> >> >> +/*
>> >> >> + * PCI-facing part of the host bridge, not usable without the
>> >> >> + * host-facing part, which can't be device_add'ed, yet.
>> >> >> + */
>> >> >> +dc->user_creatable = false;
>> >> >> +dc->vmsd = _designware_pcie_root;
>> >> >> +}
>> >> >> +
>> >> >> +static uint64_t designware_pcie_host_mmio_read(void *opaque, hwaddr 
>> >> >> addr,
>> >> >> +   unsigned int size)
>> >> >> +{
>> >> >> +PCIHostState *pci = PCI_HOST_BRIDGE(opaque);
>> >> >> +PCIDevice *device = pci_find_device(pci->bus, 0, 0);
>> >> >> +
>> >> >> +return pci_host_config_read_common(device,
>> >> >> +   addr,
>> >> >> +   pci_config_size(device),
>> >> >> +   size);
>> >> >> +}
>> >> >> +
>> >> >> +static void designware_pcie_host_mmio_write(void *opaque, hwaddr addr,
>> >> >> +uint64_t val, unsigned 
>> >> >> int size)
>> >> >> +{
>> >> >> +PCIHostState *pci = PCI_HOST_BRIDGE(opaque);
>> >> >> +PCIDevice *device = pci_find_device(pci->bus, 0, 0);
>> >> >> +
>> >> >> +return pci_host_config_write_common(device,
>> >> >> +addr,
>> >> >> +pci_config_size(device),
>> >> >> +val, size);
>> >> >> +}
>> >> >> +
>> >> >> +static const MemoryRegionOps designware_pci_mmio_ops = {
>> >> >> +.read   = designware_pcie_host_mmio_read,
>> >> >> +.write  = designware_pcie_host_mmio_write,
>> >> >> +.endianness = DEVICE_NATIVE_ENDIAN,
>> >> >> +.impl = {
>> >> >> +/*
>> >> >> + * Our device would not work correctly if the guest was doing
>> >> >> + * unaligned access. This might not be a limitation on the 
>> >> >> real
>> >> >> + * device but in practice there is no reason for a guest to 
>> >> >> access
>> >> >> + * this device unaligned.
>> >> >> + */
>> >> >> +.min_access_size = 4,
>> >> >> +.max_access_size = 4,
>> >> >> +.unaligned = false,
>> >> >> +},
>> >> >> +};
>> >> >
>> >> > Could you pls add some comments explaining why is DEVICE_NATIVE_ENDIAN
>> >> > appropriate here?  Most of these cases are plain "we never bothered
>> >> > about cross-endian setups". Some are "there's a mix of different
>> >> > endian-ness values, need to handle in a special way".
>> >> >
>> >> > I suspect you really need DEVICE_LITTLE_ENDIAN.
>> >> >
>> >>
>> >> That MemoryRegion corresponds to a register file permanently mapped
>> >> into CPU's address space, so my assumption is that SoC designers will
>> >> wire it according to CPUs endianness be it big or little. I am not
>> >> aware of any big-endian CPU based SoC on the market using Designware's
>> >> IP block, so I don't think there are any precedent confirming or
>> >> denying correctness of my assumption. IMHO, this is also the reason
>> >> why all of Linux driver code for that IP assumes little endianness.
>> >
>> > IMHO if Linux driver code does cpu_to_le then it seems best to be
>> > consistent with that.
>> >
>>
>> Well, all of the DW code does so implicitly by using readl()/writel()
>> helpers which will perform cpu_to_le/le_to_cpu under the hood. But is
>> seems to me that it could be either because the access does have to be
>> LE always or simply because readl()/writel() are goto memory helpers
>> on ARM/LE-platforms.
>
> PCI things generally tend to be LE since that's how standard
> PCI registers are defined, and being consistent avoids confusion.
>
>> FWIW: 

Re: [Qemu-devel] [PATCH v6 1/3] pci: Add support for Designware IP block

[Michael S. Tsirkin]

On Tue, Feb 13, 2018 at 02:47:24PM -0800, Andrey Smirnov wrote:
> On Tue, Feb 13, 2018 at 2:15 PM, Michael S. Tsirkin  wrote:
> > On Tue, Feb 13, 2018 at 12:24:40PM -0800, Andrey Smirnov wrote:
> >> On Tue, Feb 13, 2018 at 10:13 AM, Michael S. Tsirkin  
> >> wrote:
> >> > On Tue, Feb 13, 2018 at 09:07:10AM -0800, Andrey Smirnov wrote:
> >> >> +static void designware_pcie_root_class_init(ObjectClass *klass, void 
> >> >> *data)
> >> >> +{
> >> >> +PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >> >> +DeviceClass *dc = DEVICE_CLASS(klass);
> >> >> +
> >> >> +set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> >> >> +
> >> >> +k->vendor_id = PCI_VENDOR_ID_SYNOPSYS;
> >> >> +k->device_id = 0xABCD;
> >> >> +k->revision = 0;
> >> >> +k->class_id = PCI_CLASS_BRIDGE_PCI;
> >> >> +k->is_express = true;
> >> >> +k->is_bridge = true;
> >> >> +k->exit = pci_bridge_exitfn;
> >> >> +k->realize = designware_pcie_root_realize;
> >> >> +k->config_read = designware_pcie_root_config_read;
> >> >> +k->config_write = designware_pcie_root_config_write;
> >> >> +
> >> >> +dc->reset = pci_bridge_reset;
> >> >> +/*
> >> >> + * PCI-facing part of the host bridge, not usable without the
> >> >> + * host-facing part, which can't be device_add'ed, yet.
> >> >> + */
> >> >> +dc->user_creatable = false;
> >> >> +dc->vmsd = _designware_pcie_root;
> >> >> +}
> >> >> +
> >> >> +static uint64_t designware_pcie_host_mmio_read(void *opaque, hwaddr 
> >> >> addr,
> >> >> +   unsigned int size)
> >> >> +{
> >> >> +PCIHostState *pci = PCI_HOST_BRIDGE(opaque);
> >> >> +PCIDevice *device = pci_find_device(pci->bus, 0, 0);
> >> >> +
> >> >> +return pci_host_config_read_common(device,
> >> >> +   addr,
> >> >> +   pci_config_size(device),
> >> >> +   size);
> >> >> +}
> >> >> +
> >> >> +static void designware_pcie_host_mmio_write(void *opaque, hwaddr addr,
> >> >> +uint64_t val, unsigned int 
> >> >> size)
> >> >> +{
> >> >> +PCIHostState *pci = PCI_HOST_BRIDGE(opaque);
> >> >> +PCIDevice *device = pci_find_device(pci->bus, 0, 0);
> >> >> +
> >> >> +return pci_host_config_write_common(device,
> >> >> +addr,
> >> >> +pci_config_size(device),
> >> >> +val, size);
> >> >> +}
> >> >> +
> >> >> +static const MemoryRegionOps designware_pci_mmio_ops = {
> >> >> +.read   = designware_pcie_host_mmio_read,
> >> >> +.write  = designware_pcie_host_mmio_write,
> >> >> +.endianness = DEVICE_NATIVE_ENDIAN,
> >> >> +.impl = {
> >> >> +/*
> >> >> + * Our device would not work correctly if the guest was doing
> >> >> + * unaligned access. This might not be a limitation on the real
> >> >> + * device but in practice there is no reason for a guest to 
> >> >> access
> >> >> + * this device unaligned.
> >> >> + */
> >> >> +.min_access_size = 4,
> >> >> +.max_access_size = 4,
> >> >> +.unaligned = false,
> >> >> +},
> >> >> +};
> >> >
> >> > Could you pls add some comments explaining why is DEVICE_NATIVE_ENDIAN
> >> > appropriate here?  Most of these cases are plain "we never bothered
> >> > about cross-endian setups". Some are "there's a mix of different
> >> > endian-ness values, need to handle in a special way".
> >> >
> >> > I suspect you really need DEVICE_LITTLE_ENDIAN.
> >> >
> >>
> >> That MemoryRegion corresponds to a register file permanently mapped
> >> into CPU's address space, so my assumption is that SoC designers will
> >> wire it according to CPUs endianness be it big or little. I am not
> >> aware of any big-endian CPU based SoC on the market using Designware's
> >> IP block, so I don't think there are any precedent confirming or
> >> denying correctness of my assumption. IMHO, this is also the reason
> >> why all of Linux driver code for that IP assumes little endianness.
> >
> > IMHO if Linux driver code does cpu_to_le then it seems best to be
> > consistent with that.
> >
> 
> Well, all of the DW code does so implicitly by using readl()/writel()
> helpers which will perform cpu_to_le/le_to_cpu under the hood. But is
> seems to me that it could be either because the access does have to be
> LE always or simply because readl()/writel() are goto memory helpers
> on ARM/LE-platforms.

PCI things generally tend to be LE since that's how standard
PCI registers are defined, and being consistent avoids confusion.

> FWIW: Somewhat similar precedent of MIPS/Boston machine can serve as
> counter-example to my assumption, since Xilinx PCIE IP there seem to
> be wired to be LE despite being attached to BE CPU.
> 
> 

[Qemu-devel] [Bug 1753186] [NEW] qemu-nbd: always first snapshot loaded from VDI images with snapshots

[schmittlauch]

Public bug reported:

When mounting a virtual box disk image of a VM with snapshots, always
the state of the first snapshot is shown.

How to reproduce:
1. Create a new VirtualBox VM or use an existing one, while choosing VDI as the 
disk image format.
2. Create a snapshot of the VM.
3. Modify the file system from within the VM enough that differences to the 
snapshotted version are apparent.
4. Create another snapshot.
5. Shut down the VM.
6. Mount the partition from the disk image with `qemu-nbd -c /dev/nbd0 
/path/to/disk.vdi; mount /dev/nbd0pX /mnt`

Expected result: The mounted disk image shall represent the latest state of the 
VM
Actual result: The mounted disk image represents the VM state at the first 
snapshot

version information: qemu-nbd-2.11.1(openSUSE Tumbleweed)

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  qemu-nbd: always first snapshot loaded from VDI images with snapshots

Status in QEMU:
  New

Bug description:
  When mounting a virtual box disk image of a VM with snapshots, always
  the state of the first snapshot is shown.

  How to reproduce:
  1. Create a new VirtualBox VM or use an existing one, while choosing VDI as 
the disk image format.
  2. Create a snapshot of the VM.
  3. Modify the file system from within the VM enough that differences to the 
snapshotted version are apparent.
  4. Create another snapshot.
  5. Shut down the VM.
  6. Mount the partition from the disk image with `qemu-nbd -c /dev/nbd0 
/path/to/disk.vdi; mount /dev/nbd0pX /mnt`

  Expected result: The mounted disk image shall represent the latest state of 
the VM
  Actual result: The mounted disk image represents the VM state at the first 
snapshot

  version information: qemu-nbd-2.11.1(openSUSE Tumbleweed)

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

Re: [Qemu-devel] [PATCH v6 1/3] pci: Add support for Designware IP block

[Andrey Smirnov]

On Tue, Feb 13, 2018 at 2:47 PM, Andrey Smirnov
 wrote:
> On Tue, Feb 13, 2018 at 2:15 PM, Michael S. Tsirkin  wrote:
>> On Tue, Feb 13, 2018 at 12:24:40PM -0800, Andrey Smirnov wrote:
>>> On Tue, Feb 13, 2018 at 10:13 AM, Michael S. Tsirkin  
>>> wrote:
>>> > On Tue, Feb 13, 2018 at 09:07:10AM -0800, Andrey Smirnov wrote:
>>> >> +static void designware_pcie_root_class_init(ObjectClass *klass, void 
>>> >> *data)
>>> >> +{
>>> >> +PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>> >> +DeviceClass *dc = DEVICE_CLASS(klass);
>>> >> +
>>> >> +set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>> >> +
>>> >> +k->vendor_id = PCI_VENDOR_ID_SYNOPSYS;
>>> >> +k->device_id = 0xABCD;
>>> >> +k->revision = 0;
>>> >> +k->class_id = PCI_CLASS_BRIDGE_PCI;
>>> >> +k->is_express = true;
>>> >> +k->is_bridge = true;
>>> >> +k->exit = pci_bridge_exitfn;
>>> >> +k->realize = designware_pcie_root_realize;
>>> >> +k->config_read = designware_pcie_root_config_read;
>>> >> +k->config_write = designware_pcie_root_config_write;
>>> >> +
>>> >> +dc->reset = pci_bridge_reset;
>>> >> +/*
>>> >> + * PCI-facing part of the host bridge, not usable without the
>>> >> + * host-facing part, which can't be device_add'ed, yet.
>>> >> + */
>>> >> +dc->user_creatable = false;
>>> >> +dc->vmsd = _designware_pcie_root;
>>> >> +}
>>> >> +
>>> >> +static uint64_t designware_pcie_host_mmio_read(void *opaque, hwaddr 
>>> >> addr,
>>> >> +   unsigned int size)
>>> >> +{
>>> >> +PCIHostState *pci = PCI_HOST_BRIDGE(opaque);
>>> >> +PCIDevice *device = pci_find_device(pci->bus, 0, 0);
>>> >> +
>>> >> +return pci_host_config_read_common(device,
>>> >> +   addr,
>>> >> +   pci_config_size(device),
>>> >> +   size);
>>> >> +}
>>> >> +
>>> >> +static void designware_pcie_host_mmio_write(void *opaque, hwaddr addr,
>>> >> +uint64_t val, unsigned int 
>>> >> size)
>>> >> +{
>>> >> +PCIHostState *pci = PCI_HOST_BRIDGE(opaque);
>>> >> +PCIDevice *device = pci_find_device(pci->bus, 0, 0);
>>> >> +
>>> >> +return pci_host_config_write_common(device,
>>> >> +addr,
>>> >> +pci_config_size(device),
>>> >> +val, size);
>>> >> +}
>>> >> +
>>> >> +static const MemoryRegionOps designware_pci_mmio_ops = {
>>> >> +.read   = designware_pcie_host_mmio_read,
>>> >> +.write  = designware_pcie_host_mmio_write,
>>> >> +.endianness = DEVICE_NATIVE_ENDIAN,
>>> >> +.impl = {
>>> >> +/*
>>> >> + * Our device would not work correctly if the guest was doing
>>> >> + * unaligned access. This might not be a limitation on the real
>>> >> + * device but in practice there is no reason for a guest to 
>>> >> access
>>> >> + * this device unaligned.
>>> >> + */
>>> >> +.min_access_size = 4,
>>> >> +.max_access_size = 4,
>>> >> +.unaligned = false,
>>> >> +},
>>> >> +};
>>> >
>>> > Could you pls add some comments explaining why is DEVICE_NATIVE_ENDIAN
>>> > appropriate here?  Most of these cases are plain "we never bothered
>>> > about cross-endian setups". Some are "there's a mix of different
>>> > endian-ness values, need to handle in a special way".
>>> >
>>> > I suspect you really need DEVICE_LITTLE_ENDIAN.
>>> >
>>>
>>> That MemoryRegion corresponds to a register file permanently mapped
>>> into CPU's address space, so my assumption is that SoC designers will
>>> wire it according to CPUs endianness be it big or little. I am not
>>> aware of any big-endian CPU based SoC on the market using Designware's
>>> IP block, so I don't think there are any precedent confirming or
>>> denying correctness of my assumption. IMHO, this is also the reason
>>> why all of Linux driver code for that IP assumes little endianness.
>>
>> IMHO if Linux driver code does cpu_to_le then it seems best to be
>> consistent with that.
>>
>
> Well, all of the DW code does so implicitly by using readl()/writel()
> helpers which will perform cpu_to_le/le_to_cpu under the hood. But is
> seems to me that it could be either because the access does have to be
> LE always or simply because readl()/writel() are goto memory helpers
> on ARM/LE-platforms.
>
> FWIW: Somewhat similar precedent of MIPS/Boston machine can serve as
> counter-example to my assumption, since Xilinx PCIE IP there seem to
> be wired to be LE despite being attached to BE CPU.
>

Michael, Peter:

Just in case we are in an accidental deadlock waiting on each other,
my assumption is that this patch in particular and the rest in the
series are good as is to be applied. Please let me know if 

Re: [Qemu-devel] [PULL] RISC-V QEMU Port Submission v8

[Peter Maydell]

On 3 March 2018 at 02:46, Michael Clark  wrote:
> On Sat, Mar 3, 2018 at 3:22 AM, Peter Maydell 
> wrote:
>> Please don't send pull requests until after patches have been put
>> on list and been reviewed. A minor update to a pullreq is OK if
>> it's something like a trivial compiler fix or just dropping some
>> patches that had problems, but if you have this many changes that
>> deserves a fresh patchset to be sent to the list for review.
>>
>> (For the QEMU workflow, a pull request isn't a request for patch
>> review, it's a statement that patches have all had review and
>> are ready to go into master immediately.)
>
>
> My apoligies. I won't do this again.

No worries, it's just a workflow thing (which differs a lot from
project to project), and we don't really have much documentation
on the submaintainer part of the process. (What we do have is here:
https://wiki.qemu.org/Contribute/SubmitAPullRequest  )

The basic idea is that for us code review happens in the "patches
posted to list" phase, and then "pull request" is pretty much
the same as "commit to master". As the submaintainer you review,
test and accumulate in a branch patches from yourself and other
people, and then send them out in a pull request. In the ideal
case that goes straight into master without problems. Sometimes
it runs into trouble (like a compile issue on an oddball platform),
and then rather than going through the whole process again for
something as small as a messed up format string you can just fix
and resend the pullreq. (There are examples of this on list at
the moment, for instance.)
Bigger stuff it's usually easier to drop the relevant patches
from the pull, and then respin them and resend for review before
putting them in a later pull. The dividing line for what you
can get away with fixing up locally and what you can't is
kind of similar to what you can tweak without needing to drop
a reviewed-by: tag from a changed patch and get it re-reviewed.
When you get familiar with the process and what people do you
can take shortcuts sometimes (this is me posting what I'm
going to squash into a patch as a followup, to save reposting
a 20 patch series, for instance:
http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg00310.html).
For getting started as a submaintainer, it's probably easiest
to follow the 'by the book' process: patches go to mailing
list as 'PATCH', get review, changes made, patch series resent,
reviewed patches go into pull requests. (The idea is to ensure
that anything that goes into master has been on list for at
least a few days so people who want to review can do so.)

> I have some very very minor cleanups that do not affect logic, but perhaps
> we could address this after getting approval to make a pull request for v8.
>
> My qemu-devel branch holds changes against the latest rebase:
>
> - https://github.com/michaeljclark/riscv-qemu/tree/qemu-devel
>
> Someone raised timebase frequency on the RISC-V sw-dev and after looking at
> the code I noticed we had a hard-coded value for a few of the constants we
> put in device tree, and i spotted a missed rename. I'm going to have to
> learn about the qemu-devel process for trivial fixes...

I would probably squash in the missed rename into the relevant
patch, at any rate. The rest can probably go through the post
patch/get review/sent pull request cycle after this first lot
have been applied.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Anatoly Trosinenko]

2018-03-03 18:41 GMT+03:00 Stefan Weil :

> Am 03.03.2018 um 15:07 schrieb Anatoly Trosinenko:
> > Can rewriting TCI in such a way that every operation is aligned at 4- or
> > even 8-byte boundary fix the situation or are there some more serious
> > problems?
>
> That's my preferred solution. Are there cases which would require 8-byte
> alignment?
>

And what if create some function like

uint8_t *align_and_increment(uint8_t **ptr, int pow2) {
  size_t size = 1 << pow2;
  uint8_t *result = (uint8_t*)uintptr)*ptr) + size - 1) & ~(size - 1));
  *ptr = result + size;
  return result;
}

and rewrite get / put functions like this:

static uint32_t tci_read_i32(uint8_t **tb_ptr)
{
uint32_t value = *(uint32_t *)align_and_increment(tb_ptr, 2);
return value;
}

On one hand, it involves some slightly obscure pointer calculations
(just in one place), on the other hand, no modifications will probably
be required for TCI TCG backend or interpreter loop code (they can
still be useful for **optimizations** of bytecode size, but it should
just work as is).

--
Best regards,
Anatoly

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Stefan Weil]

Am 03.03.2018 um 15:07 schrieb Anatoly Trosinenko:
> Can rewriting TCI in such a way that every operation is aligned at 4- or
> even 8-byte boundary fix the situation or are there some more serious
> problems?

That's my preferred solution. Are there cases which would require 8-byte
alignment?

Richard, the discussion about non-portable calls to helper functions is
not new, but I still have no test case which fails. Do you think of a
special case (architecture, helper function)?

Stefan

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Anatoly Trosinenko]

> So.  Why do you want to use TCI instead of a native TCG backend?

Frankly speaking, personally I just have a strange experiment on porting
QEMU to JavaScript. :) I used the TCI bytecode as some intermediate
patchable form for rarely executing BBs and for (re)generating asm.js from
it when required. I used a Python script to generate wrappers with exactly
10 arguments around helper functions. In fact, it may be worth for me to
create WebAssembly TCG backend and interpret **that** bytecode if required.

TCI may still be useful for someone else running QEMU on unsupported host,
though.

2018-03-03 17:13 GMT+03:00 Richard Henderson :

> On 03/03/2018 06:07 AM, Anatoly Trosinenko wrote:
> > Can rewriting TCI in such a way that every operation is aligned at 4- or
> even
> > 8-byte boundary fix the situation or are there some more serious
> problems?
>
> With the current TCI, there are also problems with calls to helper
> functions.
> The only portable way to do this is to use a library such as libffi.
>
> I once rewrote TCI completely in order to address both problems, but that
> only
> brought questions as to why TCI is useful at all.
>
> So.  Why do you want to use TCI instead of a native TCG backend?
>
>
> r~
>

--
Best regards,
Anatoly

[Qemu-devel] [PATCH v4 5/5] aarch64-linux-user: Add support for SVE signal frame records

[Richard Henderson]

Depending on the currently selected size of the SVE vector registers,
we can either store the data within the "standard" allocation, or we
may beedn to allocate additional space with an EXTRA record.

Signed-off-by: Richard Henderson 
---
 linux-user/signal.c | 210 +++-
 1 file changed, 192 insertions(+), 18 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index c31cf0601d..92513f655c 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1452,6 +1452,34 @@ struct target_extra_context {
 uint32_t reserved[3];
 };
 
+#define TARGET_SVE_MAGIC0x53564501
+
+struct target_sve_context {
+struct target_aarch64_ctx head;
+uint16_t vl;
+uint16_t reserved[3];
+/* The actual SVE data immediately follows.  It is layed out
+ * according to TARGET_SVE_SIG_{Z,P}REG_OFFSET, based off of
+ * the original struct pointer.
+ */
+};
+
+#define TARGET_SVE_VQ_BYTES  16
+
+#define TARGET_SVE_SIG_ZREG_SIZE(VQ)  ((VQ) * TARGET_SVE_VQ_BYTES)
+#define TARGET_SVE_SIG_PREG_SIZE(VQ)  ((VQ) * (TARGET_SVE_VQ_BYTES / 8))
+
+#define TARGET_SVE_SIG_REGS_OFFSET \
+QEMU_ALIGN_UP(sizeof(struct target_sve_context), TARGET_SVE_VQ_BYTES)
+#define TARGET_SVE_SIG_ZREG_OFFSET(VQ, N) \
+(TARGET_SVE_SIG_REGS_OFFSET + TARGET_SVE_SIG_ZREG_SIZE(VQ) * (N))
+#define TARGET_SVE_SIG_PREG_OFFSET(VQ, N) \
+(TARGET_SVE_SIG_ZREG_OFFSET(VQ, 32) + TARGET_SVE_SIG_PREG_SIZE(VQ) * (N))
+#define TARGET_SVE_SIG_FFR_OFFSET(VQ) \
+(TARGET_SVE_SIG_PREG_OFFSET(VQ, 16))
+#define TARGET_SVE_SIG_CONTEXT_SIZE(VQ) \
+(TARGET_SVE_SIG_PREG_OFFSET(VQ, 17))
+
 struct target_rt_sigframe {
 struct target_siginfo info;
 struct target_ucontext uc;
@@ -1526,6 +1554,34 @@ static void target_setup_end_record(struct 
target_aarch64_ctx *end)
 __put_user(0, >size);
 }
 
+static void target_setup_sve_record(struct target_sve_context *sve,
+CPUARMState *env, int vq, int size)
+{
+int i, j;
+
+__put_user(TARGET_SVE_MAGIC, >head.magic);
+__put_user(size, >head.size);
+__put_user(vq * TARGET_SVE_VQ_BYTES, >vl);
+
+/* Note that SVE regs are stored as a byte stream, with each byte element
+ * at a subsequent address.  This corresponds to a little-endian store
+ * of our 64-bit hunks.
+ */
+for (i = 0; i < 32; ++i) {
+uint64_t *z = (void *)sve + TARGET_SVE_SIG_ZREG_OFFSET(vq, i);
+for (j = 0; j < vq * 2; ++j) {
+__put_user_e(env->vfp.zregs[i].d[j], z + j, le);
+}
+}
+for (i = 0; i <= 16; ++i) {
+uint16_t *p = (void *)sve + TARGET_SVE_SIG_PREG_OFFSET(vq, i);
+for (j = 0; j < vq; ++j) {
+uint64_t r = env->vfp.pregs[i].p[j >> 2];
+__put_user_e(r >> ((j & 3) * 16), p + j, le);
+}
+}
+}
+
 static void target_restore_general_frame(CPUARMState *env,
  struct target_rt_sigframe *sf)
 {
@@ -1569,14 +1625,45 @@ static void target_restore_fpsimd_record(CPUARMState 
*env,
 }
 }
 
+static void target_restore_sve_record(CPUARMState *env,
+  struct target_sve_context *sve, int vq)
+{
+int i, j;
+
+/* Note that SVE regs are stored as a byte stream, with each byte element
+ * at a subsequent address.  This corresponds to a little-endian load
+ * of our 64-bit hunks.
+ */
+for (i = 0; i < 32; ++i) {
+uint64_t *z = (void *)sve + TARGET_SVE_SIG_ZREG_OFFSET(vq, i);
+for (j = 0; j < vq * 2; ++j) {
+__get_user_e(env->vfp.zregs[i].d[j], z + j, le);
+}
+}
+for (i = 0; i <= 16; ++i) {
+uint16_t *p = (void *)sve + TARGET_SVE_SIG_PREG_OFFSET(vq, i);
+for (j = 0; j < vq; ++j) {
+uint16_t r;
+__get_user_e(r, p + j, le);
+if (j & 3) {
+env->vfp.pregs[i].p[j >> 2] |= (uint64_t)r << ((j & 3) * 16);
+} else {
+env->vfp.pregs[i].p[j >> 2] = r;
+}
+}
+}
+}
+
 static int target_restore_sigframe(CPUARMState *env,
struct target_rt_sigframe *sf)
 {
 struct target_aarch64_ctx *ctx, *extra = NULL;
 struct target_fpsimd_context *fpsimd = NULL;
+struct target_sve_context *sve = NULL;
 uint64_t extra_datap = 0;
 bool used_extra = false;
 bool err = false;
+int vq = 0, sve_size = 0;
 
 target_restore_general_frame(env, sf);
 
@@ -1608,6 +1695,18 @@ static int target_restore_sigframe(CPUARMState *env,
 fpsimd = (struct target_fpsimd_context *)ctx;
 break;
 
+case TARGET_SVE_MAGIC:
+if (arm_feature(env, ARM_FEATURE_SVE)) {
+vq = (env->vfp.zcr_el[1] & 0xf) + 1;
+sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16);
+if (!sve && size == sve_size) {
+sve = (struct 

[Qemu-devel] [PATCH v4 4/5] aarch64-linux-user: Add support for EXTRA signal frame records

[Richard Henderson]

The EXTRA record allows for additional space to be allocated
beyon what is currently reserved.  Add code to emit and read
this record type.

Nothing uses extra space yet.

Signed-off-by: Richard Henderson 
---
 linux-user/signal.c | 74 +
 1 file changed, 63 insertions(+), 11 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index f9eef3d753..c31cf0601d 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1443,6 +1443,15 @@ struct target_fpsimd_context {
 uint64_t vregs[32 * 2]; /* really uint128_t vregs[32] */
 };
 
+#define TARGET_EXTRA_MAGIC  0x45585401
+
+struct target_extra_context {
+struct target_aarch64_ctx head;
+uint64_t datap; /* 16-byte aligned pointer to extra space cast to __u64 */
+uint32_t size; /* size in bytes of the extra space */
+uint32_t reserved[3];
+};
+
 struct target_rt_sigframe {
 struct target_siginfo info;
 struct target_ucontext uc;
@@ -1502,6 +1511,15 @@ static void target_setup_fpsimd_record(struct 
target_fpsimd_context *fpsimd,
 }
 }
 
+static void target_setup_extra_record(struct target_extra_context *extra,
+  uint64_t datap, uint32_t extra_size)
+{
+__put_user(TARGET_EXTRA_MAGIC, >head.magic);
+__put_user(sizeof(struct target_extra_context), >head.size);
+__put_user(datap, >datap);
+__put_user(extra_size, >size);
+}
+
 static void target_setup_end_record(struct target_aarch64_ctx *end)
 {
 __put_user(0, >magic);
@@ -1554,48 +1572,74 @@ static void target_restore_fpsimd_record(CPUARMState 
*env,
 static int target_restore_sigframe(CPUARMState *env,
struct target_rt_sigframe *sf)
 {
-struct target_aarch64_ctx *ctx;
+struct target_aarch64_ctx *ctx, *extra = NULL;
 struct target_fpsimd_context *fpsimd = NULL;
+uint64_t extra_datap = 0;
+bool used_extra = false;
+bool err = false;
 
 target_restore_general_frame(env, sf);
 
 ctx = (struct target_aarch64_ctx *)sf->uc.tuc_mcontext.__reserved;
 while (ctx) {
-uint32_t magic, size;
+uint32_t magic, size, extra_size;
 
 __get_user(magic, >magic);
 __get_user(size, >size);
 switch (magic) {
 case 0:
 if (size != 0) {
-return 1;
+err = true;
+goto exit;
+}
+if (used_extra) {
+ctx = NULL;
+} else {
+ctx = extra;
+used_extra = true;
 }
-ctx = NULL;
 continue;
 
 case TARGET_FPSIMD_MAGIC:
 if (fpsimd || size != sizeof(struct target_fpsimd_context)) {
-return 1;
+err = true;
+goto exit;
 }
 fpsimd = (struct target_fpsimd_context *)ctx;
 break;
 
+case TARGET_EXTRA_MAGIC:
+if (extra || size != sizeof(struct target_extra_context)) {
+err = true;
+goto exit;
+}
+__get_user(extra_datap,
+   &((struct target_extra_context *)ctx)->datap);
+__get_user(extra_size,
+   &((struct target_extra_context *)ctx)->size);
+extra = lock_user(VERIFY_READ, extra_datap, extra_size, 0);
+break;
+
 default:
 /* Unknown record -- we certainly didn't generate it.
  * Did we in fact get out of sync?
  */
-return 1;
+err = true;
+goto exit;
 }
 ctx = (void *)ctx + size;
 }
 
 /* Require FPSIMD always.  */
-if (!fpsimd) {
-return 1;
+if (fpsimd) {
+target_restore_fpsimd_record(env, fpsimd);
+} else {
+err = true;
 }
-target_restore_fpsimd_record(env, fpsimd);
 
-return 0;
+ exit:
+unlock_user(extra, extra_datap, 0);
+return err;
 }
 
 static abi_ulong get_sigframe(struct target_sigaction *ka, CPUARMState *env)
@@ -1621,7 +1665,8 @@ static void target_setup_frame(int usig, struct 
target_sigaction *ka,
CPUARMState *env)
 {
 int size = offsetof(struct target_rt_sigframe, uc.tuc_mcontext.__reserved);
-int fpsimd_ofs, end1_ofs, fr_ofs;
+int fpsimd_ofs, end1_ofs, fr_ofs, end2_ofs = 0;
+int extra_ofs = 0, extra_base = 0, extra_size = 0;
 struct target_rt_sigframe *frame;
 struct target_rt_frame_record *fr;
 abi_ulong frame_addr, return_addr;
@@ -1641,7 +1686,14 @@ static void target_setup_frame(int usig, struct 
target_sigaction *ka,
 
 target_setup_general_frame(frame, env, set);
 target_setup_fpsimd_record((void *)frame + fpsimd_ofs, env);
+if (extra_ofs) {
+target_setup_extra_record((void *)frame + extra_ofs,
+  frame_addr + extra_base, extra_size);
+}
   

[Qemu-devel] [PATCH v4 1/5] linux-user: Implement aarch64 PR_SVE_SET/GET_VL

[Richard Henderson]

As an implementation choice, widening VL has zeroed the
previously inaccessible portion of the sve registers.

Reviewed-by: Peter Maydell 
Signed-off-by: Richard Henderson 
---
 linux-user/aarch64/target_syscall.h |  3 +++
 target/arm/cpu.h|  1 +
 linux-user/syscall.c| 27 
 target/arm/cpu64.c  | 41 +
 4 files changed, 72 insertions(+)

diff --git a/linux-user/aarch64/target_syscall.h 
b/linux-user/aarch64/target_syscall.h
index 604ab99b14..205265e619 100644
--- a/linux-user/aarch64/target_syscall.h
+++ b/linux-user/aarch64/target_syscall.h
@@ -19,4 +19,7 @@ struct target_pt_regs {
 #define TARGET_MLOCKALL_MCL_CURRENT 1
 #define TARGET_MLOCKALL_MCL_FUTURE  2
 
+#define TARGET_PR_SVE_SET_VL  50
+#define TARGET_PR_SVE_GET_VL  51
+
 #endif /* AARCH64_TARGET_SYSCALL_H */
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8dd6b788df..5f4566f017 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -861,6 +861,7 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, 
CPUState *cs,
 #ifdef TARGET_AARCH64
 int aarch64_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
 int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
+void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
 #endif
 
 target_ulong do_arm_semihosting(CPUARMState *env);
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e24f43c4a2..38f40e2692 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -10670,6 +10670,33 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 break;
 }
 #endif
+#ifdef TARGET_AARCH64
+case TARGET_PR_SVE_SET_VL:
+/* We cannot support either PR_SVE_SET_VL_ONEXEC
+   or PR_SVE_VL_INHERIT.  Therefore, anything above
+   ARM_MAX_VQ results in EINVAL.  */
+ret = -TARGET_EINVAL;
+if (arm_feature(cpu_env, ARM_FEATURE_SVE)
+&& arg2 >= 0 && arg2 <= ARM_MAX_VQ * 16 && !(arg2 & 15)) {
+CPUARMState *env = cpu_env;
+int old_vq = (env->vfp.zcr_el[1] & 0xf) + 1;
+int vq = MAX(arg2 / 16, 1);
+
+if (vq < old_vq) {
+aarch64_sve_narrow_vq(env, vq);
+}
+env->vfp.zcr_el[1] = vq - 1;
+ret = vq * 16;
+}
+break;
+case TARGET_PR_SVE_GET_VL:
+ret = -TARGET_EINVAL;
+if (arm_feature(cpu_env, ARM_FEATURE_SVE)) {
+CPUARMState *env = cpu_env;
+ret = ((env->vfp.zcr_el[1] & 0xf) + 1) * 16;
+}
+break;
+#endif /* AARCH64 */
 case PR_GET_SECCOMP:
 case PR_SET_SECCOMP:
 /* Disable seccomp to prevent the target disabling syscalls we
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 4228713b19..74b485b382 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -366,3 +366,44 @@ static void aarch64_cpu_register_types(void)
 }
 
 type_init(aarch64_cpu_register_types)
+
+/* The manual says that when SVE is enabled and VQ is widened the
+ * implementation is allowed to zero the previously inaccessible
+ * portion of the registers.  The corollary to that is that when
+ * SVE is enabled and VQ is narrowed we are also allowed to zero
+ * the now inaccessible portion of the registers.
+ *
+ * The intent of this is that no predicate bit beyond VQ is ever set.
+ * Which means that some operations on predicate registers themselves
+ * may operate on full uint64_t or even unrolled across the maximum
+ * uint64_t[4].  Performing 4 bits of host arithmetic unconditionally
+ * may well be cheaper than conditionals to restrict the operation
+ * to the relevant portion of a uint16_t[16].
+ *
+ * TODO: Need to call this for changes to the real system registers
+ * and EL state changes.
+ */
+void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq)
+{
+int i, j;
+uint64_t pmask;
+
+assert(vq >= 1 && vq <= ARM_MAX_VQ);
+
+/* Zap the high bits of the zregs.  */
+for (i = 0; i < 32; i++) {
+memset(>vfp.zregs[i].d[2 * vq], 0, 16 * (ARM_MAX_VQ - vq));
+}
+
+/* Zap the high bits of the pregs and ffr.  */
+pmask = 0;
+if (vq & 3) {
+pmask = ~(-1ULL << (16 * (vq & 3)));
+}
+for (j = vq / 4; j < ARM_MAX_VQ / 4; j++) {
+for (i = 0; i < 17; ++i) {
+env->vfp.pregs[i].p[j] &= pmask;
+}
+pmask = 0;
+}
+}
-- 
2.14.3

[Qemu-devel] [PATCH v4 3/5] aarch64-linux-user: Remove struct target_aux_context

[Richard Henderson]

This changes the qemu signal frame layout to be more like the kernel's,
in that the various records are dynamically allocated rather than fixed
in place by a structure.

For now, all of the allocation is out of uc.tuc_mcontext.__reserved,
so the allocation is actually trivial.  That will change with SVE support.

Reviewed-by: Peter Maydell 
Signed-off-by: Richard Henderson 
---
 linux-user/signal.c | 89 -
 1 file changed, 61 insertions(+), 28 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 25c9743aed..f9eef3d753 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1443,20 +1443,12 @@ struct target_fpsimd_context {
 uint64_t vregs[32 * 2]; /* really uint128_t vregs[32] */
 };
 
-/*
- * Auxiliary context saved in the sigcontext.__reserved array. Not exported to
- * user space as it will change with the addition of new context. User space
- * should check the magic/size information.
- */
-struct target_aux_context {
-struct target_fpsimd_context fpsimd;
-/* additional context to be added before "end" */
-struct target_aarch64_ctx end;
-};
-
 struct target_rt_sigframe {
 struct target_siginfo info;
 struct target_ucontext uc;
+};
+
+struct target_rt_frame_record {
 uint64_t fp;
 uint64_t lr;
 uint32_t tramp[2];
@@ -1562,20 +1554,47 @@ static void target_restore_fpsimd_record(CPUARMState 
*env,
 static int target_restore_sigframe(CPUARMState *env,
struct target_rt_sigframe *sf)
 {
-struct target_aux_context *aux
-= (struct target_aux_context *)sf->uc.tuc_mcontext.__reserved;
-uint32_t magic, size;
+struct target_aarch64_ctx *ctx;
+struct target_fpsimd_context *fpsimd = NULL;
 
 target_restore_general_frame(env, sf);
 
-__get_user(magic, >fpsimd.head.magic);
-__get_user(size, >fpsimd.head.size);
-if (magic == TARGET_FPSIMD_MAGIC
-&& size == sizeof(struct target_fpsimd_context)) {
-target_restore_fpsimd_record(env, >fpsimd);
-} else {
+ctx = (struct target_aarch64_ctx *)sf->uc.tuc_mcontext.__reserved;
+while (ctx) {
+uint32_t magic, size;
+
+__get_user(magic, >magic);
+__get_user(size, >size);
+switch (magic) {
+case 0:
+if (size != 0) {
+return 1;
+}
+ctx = NULL;
+continue;
+
+case TARGET_FPSIMD_MAGIC:
+if (fpsimd || size != sizeof(struct target_fpsimd_context)) {
+return 1;
+}
+fpsimd = (struct target_fpsimd_context *)ctx;
+break;
+
+default:
+/* Unknown record -- we certainly didn't generate it.
+ * Did we in fact get out of sync?
+ */
+return 1;
+}
+ctx = (void *)ctx + size;
+}
+
+/* Require FPSIMD always.  */
+if (!fpsimd) {
 return 1;
 }
+target_restore_fpsimd_record(env, fpsimd);
+
 return 0;
 }
 
@@ -1601,20 +1620,33 @@ static void target_setup_frame(int usig, struct 
target_sigaction *ka,
target_siginfo_t *info, target_sigset_t *set,
CPUARMState *env)
 {
+int size = offsetof(struct target_rt_sigframe, uc.tuc_mcontext.__reserved);
+int fpsimd_ofs, end1_ofs, fr_ofs;
 struct target_rt_sigframe *frame;
-struct target_aux_context *aux;
+struct target_rt_frame_record *fr;
 abi_ulong frame_addr, return_addr;
 
+fpsimd_ofs = size;
+size += sizeof(struct target_fpsimd_context);
+end1_ofs = size;
+size += sizeof(struct target_aarch64_ctx);
+fr_ofs = size;
+size += sizeof(struct target_rt_frame_record);
+
 frame_addr = get_sigframe(ka, env);
 trace_user_setup_frame(env, frame_addr);
 if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
 goto give_sigsegv;
 }
-aux = (struct target_aux_context *)frame->uc.tuc_mcontext.__reserved;
 
 target_setup_general_frame(frame, env, set);
-target_setup_fpsimd_record(>fpsimd, env);
-target_setup_end_record(>end);
+target_setup_fpsimd_record((void *)frame + fpsimd_ofs, env);
+target_setup_end_record((void *)frame + end1_ofs);
+
+/* Set up the stack frame for unwinding.  */
+fr = (void *)frame + fr_ofs;
+__put_user(env->xregs[29], >fp);
+__put_user(env->xregs[30], >lr);
 
 if (ka->sa_flags & TARGET_SA_RESTORER) {
 return_addr = ka->sa_restorer;
@@ -1624,13 +1656,14 @@ static void target_setup_frame(int usig, struct 
target_sigaction *ka,
  * Since these are instructions they need to be put as little-endian
  * regardless of target default or current CPU endianness.
  */
-__put_user_e(0xd2801168, >tramp[0], le);
-__put_user_e(0xd401, >tramp[1], le);
-return_addr = frame_addr + 

[Qemu-devel] [PATCH v4 2/5] aarch64-linux-user: Split out helpers for guest signal handling

[Richard Henderson]

Split out helpers from target_setup_frame and target_restore_sigframe
for dealing with general registers, fpsimd registers, and the end record.

When we add support for sve registers, the relative positions of
these will change.

Reviewed-by: Peter Maydell 
Signed-off-by: Richard Henderson 
---
 linux-user/signal.c | 120 ++--
 1 file changed, 69 insertions(+), 51 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 9a380b9e31..25c9743aed 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1462,16 +1462,17 @@ struct target_rt_sigframe {
 uint32_t tramp[2];
 };
 
-static int target_setup_sigframe(struct target_rt_sigframe *sf,
- CPUARMState *env, target_sigset_t *set)
+static void target_setup_general_frame(struct target_rt_sigframe *sf,
+   CPUARMState *env, target_sigset_t *set)
 {
 int i;
-struct target_aux_context *aux =
-(struct target_aux_context *)sf->uc.tuc_mcontext.__reserved;
 
-/* set up the stack frame for unwinding */
-__put_user(env->xregs[29], >fp);
-__put_user(env->xregs[30], >lr);
+__put_user(0, >uc.tuc_flags);
+__put_user(0, >uc.tuc_link);
+
+__put_user(target_sigaltstack_used.ss_sp, >uc.tuc_stack.ss_sp);
+__put_user(sas_ss_flags(env->xregs[31]), >uc.tuc_stack.ss_flags);
+__put_user(target_sigaltstack_used.ss_size, >uc.tuc_stack.ss_size);
 
 for (i = 0; i < 31; i++) {
 __put_user(env->xregs[i], >uc.tuc_mcontext.regs[i]);
@@ -1485,39 +1486,42 @@ static int target_setup_sigframe(struct 
target_rt_sigframe *sf,
 for (i = 0; i < TARGET_NSIG_WORDS; i++) {
 __put_user(set->sig[i], >uc.tuc_sigmask.sig[i]);
 }
+}
+
+static void target_setup_fpsimd_record(struct target_fpsimd_context *fpsimd,
+   CPUARMState *env)
+{
+int i;
+
+__put_user(TARGET_FPSIMD_MAGIC, >head.magic);
+__put_user(sizeof(struct target_fpsimd_context), >head.size);
+__put_user(vfp_get_fpsr(env), >fpsr);
+__put_user(vfp_get_fpcr(env), >fpcr);
 
 for (i = 0; i < 32; i++) {
 uint64_t *q = aa64_vfp_qreg(env, i);
 #ifdef TARGET_WORDS_BIGENDIAN
-__put_user(q[0], >fpsimd.vregs[i * 2 + 1]);
-__put_user(q[1], >fpsimd.vregs[i * 2]);
+__put_user(q[0], >vregs[i * 2 + 1]);
+__put_user(q[1], >vregs[i * 2]);
 #else
-__put_user(q[0], >fpsimd.vregs[i * 2]);
-__put_user(q[1], >fpsimd.vregs[i * 2 + 1]);
+__put_user(q[0], >vregs[i * 2]);
+__put_user(q[1], >vregs[i * 2 + 1]);
 #endif
 }
-__put_user(vfp_get_fpsr(env), >fpsimd.fpsr);
-__put_user(vfp_get_fpcr(env), >fpsimd.fpcr);
-__put_user(TARGET_FPSIMD_MAGIC, >fpsimd.head.magic);
-__put_user(sizeof(struct target_fpsimd_context),
->fpsimd.head.size);
-
-/* set the "end" magic */
-__put_user(0, >end.magic);
-__put_user(0, >end.size);
-
-return 0;
 }
 
-static int target_restore_sigframe(CPUARMState *env,
-   struct target_rt_sigframe *sf)
+static void target_setup_end_record(struct target_aarch64_ctx *end)
+{
+__put_user(0, >magic);
+__put_user(0, >size);
+}
+
+static void target_restore_general_frame(CPUARMState *env,
+ struct target_rt_sigframe *sf)
 {
 sigset_t set;
-int i;
-struct target_aux_context *aux =
-(struct target_aux_context *)sf->uc.tuc_mcontext.__reserved;
-uint32_t magic, size, fpsr, fpcr;
 uint64_t pstate;
+int i;
 
 target_to_host_sigset(, >uc.tuc_sigmask);
 set_sigmask();
@@ -1530,30 +1534,48 @@ static int target_restore_sigframe(CPUARMState *env,
 __get_user(env->pc, >uc.tuc_mcontext.pc);
 __get_user(pstate, >uc.tuc_mcontext.pstate);
 pstate_write(env, pstate);
+}
 
-__get_user(magic, >fpsimd.head.magic);
-__get_user(size, >fpsimd.head.size);
+static void target_restore_fpsimd_record(CPUARMState *env,
+ struct target_fpsimd_context *fpsimd)
+{
+uint32_t fpsr, fpcr;
+int i;
 
-if (magic != TARGET_FPSIMD_MAGIC
-|| size != sizeof(struct target_fpsimd_context)) {
-return 1;
-}
+__get_user(fpsr, >fpsr);
+vfp_set_fpsr(env, fpsr);
+__get_user(fpcr, >fpcr);
+vfp_set_fpcr(env, fpcr);
 
 for (i = 0; i < 32; i++) {
 uint64_t *q = aa64_vfp_qreg(env, i);
 #ifdef TARGET_WORDS_BIGENDIAN
-__get_user(q[0], >fpsimd.vregs[i * 2 + 1]);
-__get_user(q[1], >fpsimd.vregs[i * 2]);
+__get_user(q[0], >vregs[i * 2 + 1]);
+__get_user(q[1], >vregs[i * 2]);
 #else
-__get_user(q[0], >fpsimd.vregs[i * 2]);
-__get_user(q[1], >fpsimd.vregs[i * 2 + 1]);
+__get_user(q[0], >vregs[i * 2]);
+__get_user(q[1], >vregs[i * 2 + 1]);
 #endif
 }
-__get_user(fpsr, 

[Qemu-devel] [PATCH v4 0/5] target/arm linux-user changes for sve

[Richard Henderson]

Changes since v3:
  * Review comments applied.
  * Frame allocation generalized in patch 5; hopefully this
eliminates some of the confusion seen during review.

Changes since v2:
  * 5 patches merged,
  * The PR_SVE_SET/GET_VL patch is more specifically user-only.
  * Split the signal frame patch into 4 parts.


r~


Richard Henderson (5):
  linux-user: Implement aarch64 PR_SVE_SET/GET_VL
  aarch64-linux-user: Split out helpers for guest signal handling
  aarch64-linux-user: Remove struct target_aux_context
  aarch64-linux-user: Add support for EXTRA signal frame records
  aarch64-linux-user: Add support for SVE signal frame records

 linux-user/aarch64/target_syscall.h |   3 +
 target/arm/cpu.h|   1 +
 linux-user/signal.c | 415 ++--
 linux-user/syscall.c|  27 +++
 target/arm/cpu64.c  |  41 
 5 files changed, 418 insertions(+), 69 deletions(-)

-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Richard Henderson]

On 03/03/2018 06:07 AM, Anatoly Trosinenko wrote:
> Can rewriting TCI in such a way that every operation is aligned at 4- or even
> 8-byte boundary fix the situation or are there some more serious problems?

With the current TCI, there are also problems with calls to helper functions.
The only portable way to do this is to use a library such as libffi.

I once rewrote TCI completely in order to address both problems, but that only
brought questions as to why TCI is useful at all.

So.  Why do you want to use TCI instead of a native TCG backend?


r~

Re: [Qemu-devel] [PATCH 10/10] linux-user: init_guest_space: Try to make ARM space+commpage continuous

[Richard Henderson]

On 03/02/2018 06:13 AM, Peter Maydell wrote:
> Does anybody know why we allow the user to specify
> it on the command line? (git revision history doesn't help, it just says
> there's been a -pagesize argument since commit 54936004fddc5 in 2003,
> right back when mmap emulation was first added...)

I *think* it is to allow two different hosts with different page sizes to
achieve the same memory layout, so that tcg traces are more easily compared, to
root out bugs in the second host's tcg backend.

That's the only thing that I've used it for, anyway.


r~

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Anatoly Trosinenko]

Can rewriting TCI in such a way that every operation is aligned at 4- or
even 8-byte boundary fix the situation or are there some more serious
problems?

2018-03-03 16:57 GMT+03:00 Richard Henderson :

> On 03/03/2018 12:54 AM, Anatoly Trosinenko wrote:
> > Ping.
> > Patchwork link: http://patchwork.ozlabs.org/patch/866732/
> > 
> > Patchew link:
> > http://patchew.org/QEMU/20180127134908.24095-1-
> anatoly.trosine...@gmail.com/
> >  anatoly.trosine...@gmail.com/>
> >
> > The code in tcg/tci.c reads some data from TCI bytecode through
> > pointer dereferencing. As far as I know unaligned reads in such a way are
> > undefined behavior and compiling with -fsanitize=undefined enumerated
> > them as such at run-time.
>
> This is exactly one of the reasons why I have urged for TCI to be
> abandoned.
>
> While your patch works, it is *enormously* inefficient for hosts that
> require it.
>
>
> r~
>



-- 
С уважением,
Анатолий Тросиненко
e-mail: anatoly.trosine...@gmail.com

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Richard Henderson]

On 03/03/2018 12:54 AM, Anatoly Trosinenko wrote:
> Ping.
> Patchwork link: http://patchwork.ozlabs.org/patch/866732/
> 
> Patchew link:
> http://patchew.org/QEMU/20180127134908.24095-1-anatoly.trosine...@gmail.com/
> 
> 
> The code in tcg/tci.c reads some data from TCI bytecode through
> pointer dereferencing. As far as I know unaligned reads in such a way are
> undefined behavior and compiling with -fsanitize=undefined enumerated
> them as such at run-time.

This is exactly one of the reasons why I have urged for TCI to be abandoned.

While your patch works, it is *enormously* inefficient for hosts that require 
it.


r~

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Anatoly Trosinenko]

Ping.
Patchwork link: http://patchwork.ozlabs.org/patch/866732/
Patchew link: http://patchew.org/QEMU/20180127134908.24095-1-anatoly.
trosine...@gmail.com/

The code in tcg/tci.c reads some data from TCI bytecode through
pointer dereferencing. As far as I know unaligned reads in such a way are
undefined behavior and compiling with -fsanitize=undefined enumerated
them as such at run-time.

I have replaced such reads with invocations of ld{l,q}_he_p.
A comment in include/qemu/bswap.h:310 suggests they should be properly
translated by the compiler. I didn't added signed/unsigned casts
since bswap.h does contain separate signed/unsigned versions
for 16-bit integers but does not for 32- and 64-bit ones, so I supposed
the developers of the bswap.h already arranged everything so
integer promotions don't mess things up. I can add casts in case I'm
not right about it.
http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg06516.html

2018-01-28 9:42 GMT+03:00 Anatoly Trosinenko :

> My patch is kind of trivial quick fix that just eliminates these unaligned
> reads and doesn't seem to require complicated testing supposing my code
> properly handles integer promotion (and hope it will not slow the
> interpreter down).
>
> Aligning everything, on the other hand, can not only remove the UB but
> also speed things up, but if I get it right, requires O(opcode count)
> manual work and subsequent less trivial testing that every opcode's
> argument layout match on generation and interpretation side (errors should
> be significantly localized due to present assertion on operation size,
> though). So my patch may be considered as temporary solution.
>
> In fact, I had to similarly make these unaligned reads explicit when
> porting QEMU to JavaScript because Emscripten hugely relies on absence of
> some kinds of UB such as "implicit" unaligned accesses, and such fix seemed
> to resolve this issue for me on "host with special alignment requirement".
>
>
> 2018-01-27 19:38 GMT+03:00 Stefan Weil :
>
>> Am 27.01.2018 um 14:49 schrieb Anatoly Trosinenko:
>> > Use ldl_he_p / ldq_he_p functions instead of a plain memory access
>> > through pointer.
>> >
>> > Signed-off-by: Anatoly Trosinenko 
>> > ---
>> >  tcg/tci.c | 16 +++-
>> >  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> A better alternative might be aligning the relevant data when generating
>> the bytecode. See also my comment on alignment in tcg/tci/README.
>>
>> Stefan
>>
>>
>
>
> --
> С уважением,
> Анатолий Тросиненко
> e-mail: anatoly.trosine...@gmail.com
>



-- 
С уважением,
Анатолий Тросиненко
e-mail: anatoly.trosine...@gmail.com

[Qemu-devel] [PATCH] PPC: e500: Add check for NULL return value from qemu_find_file.

[Nia Alarie]

This prints a message and exits if the e500 BIOS firmware can't
be found, to avoid dereferencing a null pointer.

Signed-off-by: Nia Alarie 
---
 hw/ppc/e500.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index a40d3ec3e3..6ce03d6ff4 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1005,6 +1005,10 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 }
 }
 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+if (!filename) {
+error_report("Could not find firmware '%s'", bios_name);
+exit(1);
+}
 
 bios_size = load_elf(filename, NULL, NULL, _entry, , NULL,
  1, PPC_ELF_MACHINE, 0, 0);
-- 
2.16.2

[Qemu-devel] [Bug 1673976] Re: linux-user clone() can't handle glibc posix_spawn() (causes locale-gen to assert)

[Peter Maydell]

Unfortunately that won't work, because if we do a clone(CLONE_VM) in
QEMU that will mean that parent and child share not just the guest
address space, but also all the QEMU data structures for the emulated
CPUs and also the host libc data structures. Then actions done by the
child will update those data structures and break execution of the
parent when it resumes.

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

Title:
  linux-user clone() can't handle glibc posix_spawn() (causes locale-gen
  to assert)

Status in QEMU:
  New

Bug description:
  I'm running a command (locale-gen) inside of an armv7h chroot mounted
  on my x86_64 desktop by putting qemu-arm-static into /usr/bin/ of the
  chroot file system and I get a core dump.

  locale-gen
  Generating locales...
    en_US.UTF-8...localedef: ../sysdeps/unix/sysv/linux/spawni.c:360: 
__spawnix: Assertion `ec >= 0' failed.
  qemu: uncaught target signal 6 (Aborted) - core dumped
  /usr/bin/locale-gen: line 41:34 Aborted (core dumped) 
localedef -i $input -c -f $charset -A /usr/share/locale/locale.alias $locale

  I've done this same thing successfully for years, but this breakage
  has appeared some time in the last 3 or so months. Possibly with the
  update to qemu version 2.8.

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

Re: [Qemu-devel] [PATCH v4 09/11] sdcard: display protocol used when tracing

[Philippe Mathieu-Daudé]

On 02/22/2018 10:02 AM, Peter Maydell wrote:
> On 15 February 2018 at 22:05, Philippe Mathieu-Daudé  wrote:
>> put the function in sdmmc-common.c since we will reuse it in hw/sd/core.c
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
> 
> Commit message talks about a function but the patch doesn't
> add any new functions ? (Also, sentences should start with

I suppose I mistaken the commit message when I rebased, this message
belongs to the previous commit.

> a capital letter and end with a full stop, please.)

Got it!

> Otherwise
> Reviewed-by: Peter Maydell 

Thanks!

Re: [Qemu-devel] [PATCH v4 00/20] SDCard: bugfixes, support UHS-I (part 5)

[Philippe Mathieu-Daudé]

Hi Peter,

On 02/22/2018 11:31 AM, Peter Maydell wrote:
> On 15 February 2018 at 22:13, Philippe Mathieu-Daudé  wrote:
>> Some refactors, few bugfixes, better SD/SPI support.
>>
>> With this series apply, machines can use SD cards in UHS-I mode.
>> (mostly imported from Alistair Francis work)
>>
>> MMC mode split out for another series,
>> so UHS enabled MMC cards are still not usable:
>>
>>   kernel: mmc0: SDHCI controller on PCI [:00:05.0] using ADMA
>>   kernel: sd 0:0:0:0: Attached scsi generic sg0 type 0
>>   kernel: mmc0: Skipping voltage switch
>>   [mmc kthread looping]
>>
>> Since v3:
>> - simpler SPI handling, improved descriptions (Alistair review)
>> - inverted patches 16/17 order
>>
>> Since v2:
>> - split again in 2... other part is cleanup/tracing
>>
>> Since v1:
>> - rewrote mostly all patches to keep it simpler.
>>
>> $ git backport-diff
>> 001/20:[] [-C] 'sdcard: Don't always set the high capacity bit'
>> 002/20:[] [-C] 'sdcard: update the CSD CRC register regardless the CSD 
>> structure version'
>> 003/20:[] [-C] 'sdcard: fix the 'maximum data transfer rate' to 25MHz'
>> 004/20:[] [-C] 'sdcard: clean the SCR register and add few comments'
>> 005/20:[] [--] 'sdcard: remove commands from unsupported old MMC 
>> specification'
>> 006/20:[] [--] 'sdcard: simplify using the ldst API'
>> 007/20:[0008] [FC] 'sdcard: use the correct masked OCR in the R3 reply'
>> 008/20:[] [-C] 'sdcard: use the registerfields API for the CARD_STATUS 
>> register masks'
>> 009/20:[] [--] 'sdcard: handle CMD54 (SDIO)'
>> 010/20:[down] 'sdcard: handle the Security Specification commands'
>> 011/20:[down] 'sdcard: use a more descriptive label 'unimplemented_spi_cmd''
>> 012/20:[0034] [FC] 'sdcard: handles more commands in SPI mode'
>> 013/20:[] [--] 'sdcard: check the card is in correct state for APP CMD 
>> (CMD55)'
>> 014/20:[] [--] 'sdcard: warn if host uses an incorrect address for APP 
>> CMD (CMD55)'
>> 015/20:[] [--] 'sdcard: simplify SEND_IF_COND (CMD8)'
>> 016/20:[] [--] 'sdcard: simplify SD_SEND_OP_COND (ACMD41)'
>> 017/20:[] [--] 'sdcard: add SD SEND_TUNING_BLOCK (CMD19)'
>> 018/20:[] [--] 'sdcard: implement the UHS-I SWITCH_FUNCTION entries 
>> (Spec v3)'
>> 019/20:[] [-C] 'sdcard: add a 'uhs' property, update the OCR register 
>> ACCEPT_SWITCH_1V8 bit'
>> 020/20:[] [--] 'sdcard: add an enum for the SD PHY Spec version'
> 
> I've applied patches 1 to 16 to target-arm.next.

Thanks!

> Removing the CMD11 support worries me a bit -- presumably it was put there
> because some guest actually uses it -- but my test images seem to
> still boot OK...

Yes you are right, this command was available in the first MMC specs but
not included in the SD specs. The SD/MMC differences are not always easy
to track and implement, the last series is supposed to handle MMC in a
clearer way, I'll see to restore this command once there.

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v4 10/20] sdcard: handle the Security Specification commands

[Philippe Mathieu-Daudé]

On 02/15/2018 07:54 PM, Alistair Francis wrote:
> On Thu, Feb 15, 2018 at 2:13 PM, Philippe Mathieu-Daudé  
> wrote:
>> returning sd_illegal, since they are not implemented.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  hw/sd/sd.c | 11 +++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 30acd04ad7..0457f5214b 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -1551,6 +1551,17 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>>  }
>>  break;
>>
>> +case 18:/* Reserved for SD security applications */
>> +case 25:
>> +case 26:
>> +case 38:
>> +case 43 ... 49:
>> +/* Refer to the "SD Specifications Part3 Security Specification" for
>> + * information about the SD Security Features */
> 
> The */ should be on a new line.

Thanks Peter for fixing this :)

> 
> Otherwise:
> 
> Reviewed-by: Alistair Francis 

Thanks!

> 
> Alistair
> 
>> +qemu_log_mask(LOG_UNIMP, "SD: CMD%i Security not implemented\n",
>> +  req.cmd);
>> +return sd_illegal;
>> +
>>  default:
>>  /* Fall back to standard commands.  */
>>  return sd_normal_command(sd, req);
>> --
>> 2.16.1
>>
>>

Re: [Qemu-devel] [PATCH v4 20/20] sdcard: add an enum for the SD PHY Spec version

[Philippe Mathieu-Daudé]

On 02/22/2018 11:26 AM, Peter Maydell wrote:
> On 15 February 2018 at 22:13, Philippe Mathieu-Daudé  wrote:
>> So far this device intends to model the Spec v1.10
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> Reviewed-by: Alistair Francis 
>> ---
>>  hw/sd/sd.c | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index b9429b06ca..d4565626ce 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -47,6 +47,11 @@
>>
>>  //#define DEBUG_SD 1
>>
>> +typedef enum {
>> +SD_PHY_SPEC_VER_1_10 = 110,
>> +SD_PHY_SPEC_VER_2_00 = 200, /* not yet supported */
>> +} sd_phy_spec_ver_t;
>> +
>>  typedef enum {
>>  sd_r0 = 0,/* no response */
>>  sd_r1,/* normal response command */
>> @@ -122,6 +127,7 @@ struct SDState {
>>  qemu_irq inserted_cb;
>>  QEMUTimer *ocr_power_timer;
>>  const char *proto_name;
>> +int spec_version;
>>  bool enable;
>>  uint8_t dat_lines;
>>  bool cmd_line;
>> @@ -2169,6 +2175,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
>>  int ret;
>>
>>  sd->proto_name = sd->spi ? "SPI" : "SD";
>> +sd->spec_version = SD_PHY_SPEC_VER_1_10;
>>
>>  if (sd->blk && blk_is_read_only(sd->blk)) {
>>  error_setg(errp, "Cannot use read-only drive as SD card");
> 
> I think I'd prefer to see this patch with the ones that actually use
> the field -- as it stands it's never used.

Indeed I see, thanks.

Re: [Qemu-devel] [PATCH v4 19/20] sdcard: add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit

[Philippe Mathieu-Daudé]

Hi Peter,

On 02/22/2018 11:25 AM, Peter Maydell wrote:
> On 15 February 2018 at 22:13, Philippe Mathieu-Daudé  wrote:
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  hw/sd/sd.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index ada96f5574..b9429b06ca 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -93,6 +93,7 @@ struct SDState {
>>  /* Configurable properties */
>>  BlockBackend *blk;
>>  bool spi;
>> +uint8_t uhs_supported;
>>
>>  uint32_t mode;/* current card mode, one of SDCardModes */
>>  int32_t state;/* current card state, one of SDCardStates */
>> @@ -292,6 +293,8 @@ static void sd_set_ocr(SDState *sd)
>>  {
>>  /* All voltages OK */
>>  sd->ocr = R_OCR_VDD_VOLTAGE_WIN_HI_MASK;
>> +
>> +sd->ocr = FIELD_DP32(sd->ocr, OCR, ACCEPT_SWITCH_1V8, 
>> !!sd->uhs_supported);
>>  }
>>
>>  static void sd_ocr_powerup(void *opaque)
>> @@ -2189,6 +2192,7 @@ static Property sd_properties[] = {
>>   * board to ensure that ssi transfers only occur when the chip select
>>   * is asserted.  */
>>  DEFINE_PROP_BOOL("spi", SDState, spi, false),
>> +DEFINE_PROP_UINT8("uhs", SDState, uhs_supported, UHS_NOT_SUPPORTED),
>>  DEFINE_PROP_END_OF_LIST()
>>  };
> 
> The field name and the use of the value suggest that this is just
> a boolean flag, so why define it as a PROP_UINT8 rather than PROP_BOOL ?

Here is the full enum:

typedef enum  {
UHS_NOT_SUPPORTED   = 0,
UHS_I   = 1,
UHS_II  = 2,/* currently not supported */
UHS_III = 3,/* currently not supported */
} sd_uhs_mode_t;

I'll rename it "uhs_mode" to reflect this is not a boolean.

Some controllers modeled support UHS-II, I guess we'll need to implement
it at some point during the year.

Regards,

Phil.