* [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
From: Gabriele Paoloni @ 2016-09-22 11:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3538381.vOsx75UXVU@wuerfel>
Hi Arnd
> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> Sent: 21 September 2016 21:18
> To: Gabriele Paoloni
> Cc: zhichang; linux-arm-kernel at lists.infradead.org;
> devicetree at vger.kernel.org; lorenzo.pieralisi at arm.com; minyard at acm.org;
> linux-pci at vger.kernel.org; gregkh at linuxfoundation.org; John Garry;
> will.deacon at arm.com; linux-kernel at vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-serial at vger.kernel.org;
> benh at kernel.crashing.org; zourongrong at gmail.com; liviu.dudau at arm.com;
> kantyzc at 163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
>
> On Wednesday, September 21, 2016 4:20:55 PM CEST Gabriele Paoloni
> wrote:
> > > -----Original Message-----
> > > From: zhichang [mailto:zhichang.yuan02 at gmail.com]
> > > On 2016?09?15? 20:24, Arnd Bergmann wrote:
> > > > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni
> > > wrote:
> > > >>> -----Original Message-----
> > > >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele
> Paoloni
> > > wrote:
> > > >> I think that maybe having the 1:1 range mapping doesn't
> > > >> reflect well the reality but it is the less painful
> > > >> solution...
> > > >>
> > > >> What's your view?
> > > >
> > > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags,
> > > > and that should be enough to translate the I/O port number.
> > > >
> > > > The only part we need to change here is to not go through
> > > > the crazy conversion all the way from PCI I/O space to a
> > > > physical address and back to a (logical) port number
> > > > that we do today with of_translate_address/pci_address_to_pio.
> > > >
> > > Sorry for the late response! Several days' leave....
> > > Do you want to bypass of_translate_address and pci_address_to_pio
> for
> > > the registered specific PIO?
> > > I think the bypass for of_translate_address is ok, but worry some
> new
> > > issues will emerge without the
> > > conversion between physical address and logical/linux port number.
>
> The same function that handles the non-translated region would
> do that conversion.
>
> > > When PCI host bridge which support IO operations is configured and
> > > enabled, the pci_address_to_pio will
> > > populate the logical IO range from ZERO for the first host bridge.
> Our
> > > LPC will also use part of the IO range
> > > started from ZERO. It will make in/out enter the wrong branch
> possibly.
> > >
> > > In V2, the 0 - 0x1000 logical IO range is reserved for LPC use
> only.
> > > But it seems not so good. In this way,
> > > PCI has no chance to use low 4K IO range(logical).
> > >
> > > So, in V3, applying the conversion from physical/cpu address to
> > > logical/linux IO port for any IO ranges,
> > > including the LPC, but recorded the logical IO range for LPC. When
> > > calling in/out with a logical port address,
> > > we can check this port fall into LPC logical IO range and get back
> the
> > > real IO.
>
> Right, and the same translation can be used in
> __of_address_to_resource()
> going the opposite way.
>
> > > Do you have further comments about this??
> >
> > I think there are two separate issues to be discussed:
> >
> > The first issue is about having of_translate_address failing due to
> > "range" missing. About this Arnd suggested that it is not appropriate
> > to have a range describing a bridge 1:1 mapping and this was
> discussed
> > before in this thread. Arnd had a suggestion about this (see below)
> > however (looking twice at the code) it seems to me that such solution
> > would lead to quite some duplication from __of_translate_address()
> > in order to retrieve the actual addr from dt...
>
> I don't think we need to duplicate much, we can probably safely
> assume that there are no nontrivial ranges in devices below the LPC
> node, so we just walk up the bus to see if the node is a child
> (or possibly grandchild etc) of the LPC bus, and treat any IO port
> number under there as a physical port number, which has a known
> offset from the Linux I/O port number.
>
> > I think extending of_empty_ranges_quirk() may be a reasonable
> solution.
> > What do you think Arnd?
>
> I don't really like that idea, that quirk is meant to work around
> broken DTs, but we can just make the DT valid and implement the
> code properly.
Ok I understand your point where it is not right to use of_empty_ranges_quirk()
As a quirk is used to work around broken HW or broken FW (as in this case)
rather than to fix code
What about the following? I think adding the check you suggested next to
of_empty_ranges_quirk() is adding the case we need in the right point (thus
avoiding any duplication)
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct device_node *np)
return NULL;
}
+static inline int of_isa_indirect_io(struct device_node *np)
+{
+ /*
+ * check if the current node is an isa bus and if indirectio operation
+ * are registered
+ */
+ return (of_bus_isa_match(np) && arm64_extio_ops);
+}
+
static int of_empty_ranges_quirk(struct device_node *np)
{
if (IS_ENABLED(CONFIG_PPC)) {
@@ -503,7 +512,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
* This code is only enabled on powerpc. --gcl
*/
ranges = of_get_property(parent, rprop, &rlen);
- if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
+ if (ranges == NULL && !of_empty_ranges_quirk(parent) && !of_isa_indirect_io(parent)) {
pr_debug("OF: no ranges; cannot translate\n");
return 1;
}
>
> > The second issue is a conflict between cpu addresses used by the LPC
> > controller and i/o tokens from pci endpoints.
> >
> > About this what if we modify armn64_extio_ops to have a list of
> ranges
> > rather than only one range (now we have just start/end); then in the
> > LPC driver we can scan the LPC child devices and
> > 1) populate such list of ranges
> > 2) call pci_register_io_range for such ranges
>
> Scanning the child devices sounds really wrong, please register just
> one range that covers the bus to keep the workaround as simple
> as possible.
>
> > Then when calling __of_address_to_resource we retrieve I/O tokens
> > for the devices on top of the LPC driver and in the I/O accessors
> > we call pci_pio_to_address to figure out the cpu address and compare
> > it to the list of ranges in armn64_extio_ops.
> >
> > What about this?
>
> That seems really complex for something that can be quite simple.
> The only thing we need to worry about is that the io_range_list
> contains an entry for the LPC bus so we don't conflict with the
> PCI buses.
Thanks
I discussed with Zhichang and we agreed to use only one LPC range
to be registered with pci_register_io_range.
We'll rework the accessors to check if the retrieved I/O tokens
belong to LPC or PCI IO range...
Cheers
Gab
>
> Arnd
>
> Arnd
^ permalink raw reply
* [PATCH v6 1/7] arm/arm64: vgic-new: Implement support for userspace access
From: Marc Zyngier @ 2016-09-22 12:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474351965-11586-2-git-send-email-vijay.kilari@gmail.com>
On 20/09/16 07:12, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> Read and write of some registers like ISPENDR and ICPENDR
> from userspace requires special handling when compared to
> guest access for these registers.
>
> Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> for handling of ISPENDR, ICPENDR registers handling.
>
> Add infrastructure to support guest and userspace read
> and write for the required registers
> Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 ----------
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 98 ++++++++++++++++++++++++++++++++--------
> virt/kvm/arm/vgic/vgic-mmio.c | 78 ++++++++++++++++++++++++++++----
> virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
> 4 files changed, 169 insertions(+), 51 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index b44b359..0b32f40 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -406,31 +406,6 @@ int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> return -ENXIO;
> }
>
> -/*
> - * When userland tries to access the VGIC register handlers, we need to
> - * create a usable struct vgic_io_device to be passed to the handlers and we
> - * have to set up a buffer similar to what would have happened if a guest MMIO
> - * access occurred, including doing endian conversions on BE systems.
> - */
> -static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> - bool is_write, int offset, u32 *val)
> -{
> - unsigned int len = 4;
> - u8 buf[4];
> - int ret;
> -
> - if (is_write) {
> - vgic_data_host_to_mmio_bus(buf, len, *val);
> - ret = kvm_io_gic_ops.write(vcpu, &dev->dev, offset, len, buf);
> - } else {
> - ret = kvm_io_gic_ops.read(vcpu, &dev->dev, offset, len, buf);
> - if (!ret)
> - *val = vgic_data_mmio_bus_to_host(buf, len);
> - }
> -
> - return ret;
> -}
> -
> int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> int offset, u32 *val)
> {
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 0d3c76a..ce2708d 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -209,6 +209,62 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> +static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + u32 value = 0;
> + int i;
> +
> + /*
> + * A level triggerred interrupt pending state is latched in both
> + * "soft_pending" and "line_level" variables. Userspace will save
> + * and restore soft_pending and line_level separately.
> + * Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> + * handling of ISPENDR and ICPENDR.
> + */
> + for (i = 0; i < len * 8; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> + if (irq->config == VGIC_CONFIG_LEVEL && irq->soft_pending)
> + value |= (1U << i);
> + if (irq->config == VGIC_CONFIG_EDGE && irq->pending)
> + value |= (1U << i);
> +
> + vgic_put_irq(vcpu->kvm, irq);
> + }
> +
> + return value;
> +}
> +
> +static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len,
> + unsigned long val)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + int i;
> +
> + for (i = 0; i < len * 8; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> + spin_lock(&irq->irq_lock);
> + if (test_bit(i, &val)) {
> + irq->pending = true;
> + irq->soft_pending = true;
> + vgic_queue_irq_unlock(vcpu->kvm, irq);
> + } else {
> + irq->soft_pending = false;
> + if (irq->config == VGIC_CONFIG_EDGE ||
> + (irq->config == VGIC_CONFIG_LEVEL &&
> + !irq->line_level))
> + irq->pending = false;
> + spin_unlock(&irq->irq_lock);
> + }
> +
> + vgic_put_irq(vcpu->kvm, irq);
> + }
> +}
> +
> /* We want to avoid outer shareable. */
> u64 vgic_sanitise_shareability(u64 field)
> {
> @@ -358,7 +414,7 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
> * We take some special care here to fix the calculation of the register
> * offset.
> */
> -#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, bpi, acc) \
> +#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, ur, uw, bpi, acc) \
> { \
> .reg_offset = off, \
> .bits_per_irq = bpi, \
> @@ -373,6 +429,8 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
> .access_flags = acc, \
> .read = rd, \
> .write = wr, \
> + .uaccess_read = ur, \
> + .uaccess_write = uw, \
> }
>
> static const struct vgic_register_region vgic_v3_dist_registers[] = {
> @@ -380,40 +438,42 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
> - vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
> + vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
> - vgic_mmio_read_enable, vgic_mmio_write_senable, 1,
> + vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICENABLER,
> - vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
> + vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
> - vgic_mmio_read_pending, vgic_mmio_write_spending, 1,
> + vgic_mmio_read_pending, vgic_mmio_write_spending,
> + vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 1,
> + vgic_mmio_read_pending, vgic_mmio_write_cpending,
> + vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> - vgic_mmio_read_active, vgic_mmio_write_sactive, 1,
> + vgic_mmio_read_active, vgic_mmio_write_sactive, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
> - vgic_mmio_read_active, vgic_mmio_write_cactive, 1,
> + vgic_mmio_read_active, vgic_mmio_write_cactive, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
> - vgic_mmio_read_priority, vgic_mmio_write_priority, 8,
> - VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
> + vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> + 8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ITARGETSR,
> - vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
> + vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8,
> VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICFGR,
> - vgic_mmio_read_config, vgic_mmio_write_config, 2,
> + vgic_mmio_read_config, vgic_mmio_write_config, NULL, NULL, 2,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGRPMODR,
> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> + vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IROUTER,
> - vgic_mmio_read_irouter, vgic_mmio_write_irouter, 64,
> + vgic_mmio_read_irouter, vgic_mmio_write_irouter, NULL, NULL, 64,
> VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GICD_IDREGS,
> vgic_mmio_read_v3_idregs, vgic_mmio_write_wi, 48,
> @@ -451,11 +511,13 @@ static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
> REGISTER_DESC_WITH_LENGTH(GICR_ICENABLER0,
> vgic_mmio_read_enable, vgic_mmio_write_cenable, 4,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_LENGTH(GICR_ISPENDR0,
> - vgic_mmio_read_pending, vgic_mmio_write_spending, 4,
> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ISPENDR0,
> + vgic_mmio_read_pending, vgic_mmio_write_spending,
> + vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_LENGTH(GICR_ICPENDR0,
> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 4,
> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ICPENDR0,
> + vgic_mmio_read_pending, vgic_mmio_write_cpending,
> + vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GICR_ISACTIVER0,
> vgic_mmio_read_active, vgic_mmio_write_sactive, 4,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index e18b30d..31f85df 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -468,6 +468,73 @@ static bool check_region(const struct vgic_register_region *region,
> return false;
> }
>
> +static const struct vgic_register_region *
> +vgic_get_mmio_region(struct vgic_io_device *iodev, gpa_t addr, int len)
> +{
> + const struct vgic_register_region *region;
> +
> + region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> + addr - iodev->base_addr);
> + if (!region || !check_region(region, addr, len))
> + return NULL;
> +
> + return region;
> +}
> +
> +static int vgic_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> + gpa_t addr, u32 *val)
> +{
> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
> + const struct vgic_register_region *region;
> + struct kvm_vcpu *r_vcpu;
> +
> + region = vgic_get_mmio_region(iodev, addr, sizeof(u32));
> + if (!region) {
> + *val = 0;
> + return 0;
This is not the previous semantic of vgic_uaccess, and I cannot see why
blindly ignoring an access to an undefined region would be acceptable.
What am I missing?
> + }
> +
> + r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
> + if (region->uaccess_read)
> + *val = region->uaccess_read(r_vcpu, addr, sizeof(u32));
> + else
> + *val = region->read(r_vcpu, addr, sizeof(u32));
> +
> + return 0;
> +}
> +
> +static int vgic_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> + gpa_t addr, const u32 *val)
> +{
> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
> + const struct vgic_register_region *region;
> + struct kvm_vcpu *r_vcpu;
> +
> + region = vgic_get_mmio_region(iodev, addr, sizeof(u32));
> + if (!region)
> + return 0;
Same here.
> +
> + r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
> + if (region->uaccess_write)
> + region->uaccess_write(r_vcpu, addr, sizeof(u32), *val);
> + else
> + region->write(r_vcpu, addr, sizeof(u32), *val);
> +
> + return 0;
> +}
> +
> +/*
> + * Userland access to VGIC registers.
> + */
> +int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> + bool is_write, int offset, u32 *val)
> +{
> + if (is_write)
> + return vgic_uaccess_write(vcpu, &dev->dev, offset, val);
> + else
> + return vgic_uaccess_read(vcpu, &dev->dev, offset, val);
> +}
> +
> static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> gpa_t addr, int len, void *val)
> {
> @@ -475,9 +542,8 @@ static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> const struct vgic_register_region *region;
> unsigned long data = 0;
>
> - region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> - addr - iodev->base_addr);
> - if (!region || !check_region(region, addr, len)) {
> + region = vgic_get_mmio_region(iodev, addr, len);
> + if (!region) {
> memset(val, 0, len);
> return 0;
> }
> @@ -508,14 +574,10 @@ static int dispatch_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> const struct vgic_register_region *region;
> unsigned long data = vgic_data_mmio_bus_to_host(val, len);
>
> - region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> - addr - iodev->base_addr);
> + region = vgic_get_mmio_region(iodev, addr, len);
> if (!region)
> return 0;
>
> - if (!check_region(region, addr, len))
> - return 0;
> -
> switch (iodev->iodev_type) {
> case IODEV_CPUIF:
> region->write(vcpu, addr, len, data);
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index 4c34d39..97e6df7 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -34,6 +34,10 @@ struct vgic_register_region {
> gpa_t addr, unsigned int len,
> unsigned long val);
> };
> + unsigned long (*uaccess_read)(struct kvm_vcpu *vcpu, gpa_t addr,
> + unsigned int len);
> + void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
> + unsigned int len, unsigned long val);
> };
>
> extern struct kvm_io_device_ops kvm_io_gic_ops;
> @@ -86,6 +90,18 @@ extern struct kvm_io_device_ops kvm_io_gic_ops;
> .write = wr, \
> }
>
> +#define REGISTER_DESC_WITH_LENGTH_UACCESS(off, rd, wr, urd, uwr, length, acc) \
> + { \
> + .reg_offset = off, \
> + .bits_per_irq = 0, \
> + .len = length, \
> + .access_flags = acc, \
> + .read = rd, \
> + .write = wr, \
> + .uaccess_read = urd, \
> + .uaccess_write = uwr, \
> + }
> +
> int kvm_vgic_register_mmio_region(struct kvm *kvm, struct kvm_vcpu *vcpu,
> struct vgic_register_region *reg_desc,
> struct vgic_io_device *region,
> @@ -158,6 +174,9 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val);
>
> +int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> + bool is_write, int offset, u32 *val);
> +
> unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
>
> unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev);
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
From: Arnd Bergmann @ 2016-09-22 12:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E1F881744@lhreml507-mbx>
On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni wrote:
> > > I think extending of_empty_ranges_quirk() may be a reasonable
> > solution.
> > > What do you think Arnd?
> >
> > I don't really like that idea, that quirk is meant to work around
> > broken DTs, but we can just make the DT valid and implement the
> > code properly.
>
> Ok I understand your point where it is not right to use of_empty_ranges_quirk()
> As a quirk is used to work around broken HW or broken FW (as in this case)
> rather than to fix code
>
> What about the following? I think adding the check you suggested next to
> of_empty_ranges_quirk() is adding the case we need in the right point (thus
> avoiding any duplication)
>
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct device_node *np)
> return NULL;
> }
>
> +static inline int of_isa_indirect_io(struct device_node *np)
> +{
> + /*
> + * check if the current node is an isa bus and if indirectio operation
> + * are registered
> + */
> + return (of_bus_isa_match(np) && arm64_extio_ops);
> +}
> +
> static int of_empty_ranges_quirk(struct device_node *np)
> {
> if (IS_ENABLED(CONFIG_PPC)) {
> @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
> * This code is only enabled on powerpc. --gcl
> */
> ranges = of_get_property(parent, rprop, &rlen);
> - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> + if (ranges == NULL && !of_empty_ranges_quirk(parent) && !of_isa_indirect_io(parent)) {
> pr_debug("OF: no ranges; cannot translate\n");
> return 1;
> }
I don't see what effect that would have. What do you want to
achieve with this?
I think all we need from this function is to return '1' if
we hit an ISA I/O window, and that should happen for the two
interesting cases, either no 'ranges' at all, or no translation
for the range in question, so that __of_translate_address can
return OF_BAD_ADDR, and we can enter the special case
handling in the caller, that handles it like
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903fe9d2..a18d96843fae 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -685,17 +685,24 @@ static int __of_address_to_resource(struct device_node *dev,
if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
return -EINVAL;
taddr = of_translate_address(dev, addrp);
- if (taddr == OF_BAD_ADDR)
- return -EINVAL;
memset(r, 0, sizeof(struct resource));
+
if (flags & IORESOURCE_IO) {
unsigned long port;
- port = pci_address_to_pio(taddr);
+
+ if (taddr == OF_BAD_ADDR)
+ port = arch_of_address_to_pio(dev, addrp)
+ else
+ port = pci_address_to_pio(taddr);
+
if (port == (unsigned long)-1)
return -EINVAL;
r->start = port;
r->end = port + size - 1;
} else {
+ if (taddr == OF_BAD_ADDR)
+ return -EINVAL;
+
r->start = taddr;
r->end = taddr + size - 1;
}
Arnd
^ permalink raw reply related
* [PATCH v6 6/7] arm/arm64: vgic-new: Implement VGICv3 CPU interface access
From: Marc Zyngier @ 2016-09-22 12:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474351965-11586-7-git-send-email-vijay.kilari@gmail.com>
On 20/09/16 07:12, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> VGICv3 CPU interface registers are accessed using
> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
> as 64-bit. The cpu MPIDR value is passed along with register id.
> is used to identify the cpu for registers access.
>
> The version of VGIC v3 specification is define here
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> arch/arm64/include/uapi/asm/kvm.h | 3 +
> arch/arm64/kvm/Makefile | 1 +
> include/kvm/arm_vgic.h | 9 +
> virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
> virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 327 ++++++++++++++++++++++++++++++++++++
> virt/kvm/arm/vgic/vgic-v3.c | 7 +
> virt/kvm/arm/vgic/vgic.h | 4 +
> 8 files changed, 397 insertions(+)
>
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 56dc08d..91c7137 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
> (0xffffffffULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
> #define KVM_DEV_ARM_VGIC_OFFSET_SHIFT 0
> #define KVM_DEV_ARM_VGIC_OFFSET_MASK (0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
> +#define KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0xffff)
> #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
> #define KVM_DEV_ARM_VGIC_GRP_CTRL 4
> #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6
> +
> #define KVM_DEV_ARM_VGIC_CTRL_INIT 0
>
> /* Device Control API on vcpu fd */
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index d50a82a..1a14e29 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
> kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 002f092..b986c25 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -71,6 +71,15 @@ struct vgic_global {
>
> /* GIC system register CPU interface */
> struct static_key_false gicv3_cpuif;
> +
> + /* Cache ICH_VTR_EL2 reg value */
> + u32 ich_vtr_el2;
> +
> + /* Cache guest priority bits */
> + u32 num_pri_bits;
> +
> + /* Cache guest interrupt ID bits */
> + u32 num_id_bits;
I'm going to cry now... Have you noticed that you are caching
guest-dependent values in a global structure? What happens when you have
more than one?
Caching the *host* values are fine, but the guest's? Come on...
> };
>
> extern struct vgic_global kvm_vgic_global_state;
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 6c7d30c..da532d1 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -504,6 +504,14 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> if (!is_write)
> *reg = tmp32;
> break;
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> + u64 regid;
> +
> + regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
> + ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
> + regid, reg);
> + break;
> + }
> default:
> ret = -EINVAL;
> break;
> @@ -537,6 +545,15 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
> reg = tmp32;
> return vgic_v3_attr_regs_access(dev, attr, ®, true);
> }
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> + u64 reg;
> +
> + if (get_user(reg, uaddr))
> + return -EFAULT;
> +
> + return vgic_v3_attr_regs_access(dev, attr, ®, true);
> + }
> }
> return -ENXIO;
> }
> @@ -563,6 +580,15 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
> tmp32 = reg;
> return put_user(tmp32, uaddr);
> }
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> + u64 reg;
> +
> + ret = vgic_v3_attr_regs_access(dev, attr, ®, false);
> + if (ret)
> + return ret;
> + return put_user(reg, uaddr);
> + }
> }
>
> return -ENXIO;
> @@ -581,6 +607,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> break;
> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
> return vgic_v3_has_attr_regs(dev, attr);
> case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
> return 0;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index b35fb83..8f46e61 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -23,6 +23,7 @@
>
> #include "vgic.h"
> #include "vgic-mmio.h"
> +#include "sys_regs.h"
>
> /* extract @num bytes at @offset bytes offset in data */
> unsigned long extract_bytes(u64 data, unsigned int offset,
> @@ -639,6 +640,24 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
> break;
> }
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> + u64 reg, id;
> + unsigned long vgic_mpidr, mpidr_reg;
> + struct kvm_vcpu *vcpu;
> +
> + vgic_mpidr = (attr->attr & KVM_DEV_ARM_VGIC_V3_MPIDR_MASK) >>
> + KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT;
> +
> + /* Convert plain mpidr value to MPIDR reg format */
> + mpidr_reg = VGIC_TO_MPIDR(mpidr_reg);
> +
> + vcpu = kvm_mpidr_to_vcpu(dev->kvm, mpidr_reg);
> + if (!vcpu)
> + return -EINVAL;
> +
> + id = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
> + return vgic_v3_has_cpu_sysregs_attr(vcpu, 0, id, ®);
> + }
> default:
> return -ENXIO;
> }
> diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> new file mode 100644
> index 0000000..769920f
> --- /dev/null
> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> @@ -0,0 +1,327 @@
> +#include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/kvm.h>
> +#include <linux/kvm_host.h>
> +#include <kvm/iodev.h>
> +#include <kvm/arm_vgic.h>
> +#include <asm/kvm_emulate.h>
> +#include <asm/kvm_arm.h>
> +#include <asm/kvm_mmu.h>
> +
> +#include "vgic.h"
> +#include "vgic-mmio.h"
> +#include "sys_regs.h"
> +
> +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_vmcr vmcr;
> + u64 val;
> + u32 hw_pri_bits, num_pri_bits, hw_id_bits, num_id_bits;
> +
> + vgic_get_vmcr(vcpu, &vmcr);
> + if (p->is_write) {
> + val = p->regval;
> +
> + /*
> + * Does not allow update of ICC_CTLR_EL1 if HW does not support
> + * guest programmed ID and PRI bits
> + */
> + hw_pri_bits = ((kvm_vgic_global_state.ich_vtr_el2 &
> + ICH_VTR_PRI_BITS_MASK) >>
> + ICH_VTR_PRI_BITS_SHIFT) + 1;
> + num_pri_bits = ((val & ICC_CTLR_EL1_PRI_BITS_MASK) >>
> + ICC_CTLR_EL1_PRI_BITS_SHIFT) + 1;
> + if (num_pri_bits > hw_pri_bits)
> + return false;
> +
> + kvm_vgic_global_state.num_pri_bits = num_pri_bits;
And here we go...
I've stopped reading here, because you can't possibly have tested this.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH 0/2] BQL support and fix for a regression issue
From: David Miller @ 2016-09-22 12:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474535121-13958-1-git-send-email-sunil.kovvuri@gmail.com>
From: sunil.kovvuri@gmail.com
Date: Thu, 22 Sep 2016 14:35:19 +0530
> From: Sunil Goutham <sgoutham@cavium.com>
>
> These patches add byte queue limit support and also fixes a regression
> issue introduced by commit
> 'net: thunderx: Use netdev's name for naming VF's interrupts'
The correct way to refer to commits when you fix a bug is
to use the "Fixes: " tag right before the signoffs and ACKs.
The correct form is:
Fixes: $SHA1_ID ("Commit header line.")
You'll see this in many other netdev commits.
^ permalink raw reply
* [PATCH v4 2/2] KVM: arm/arm64: Route vtimer events to user space
From: Alexander Graf @ 2016-09-22 12:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57E0FF95.7040305@arm.com>
On 09/20/2016 11:21 AM, Marc Zyngier wrote:
> On 19/09/16 18:39, Alexander Graf wrote:
>>
>> On 19.09.16 16:48, Marc Zyngier wrote:
>>
>>>> +
>>>> + /*
>>>> + * So we can just explicitly mask or unmask the IRQ, gaining
>>>> + * more compatibility with oddball irq controllers.
>>>> + */
>>>> + if (phys_active)
>>>> + disable_percpu_irq(host_vtimer_irq);
>>>> + else
>>>> + enable_percpu_irq(host_vtimer_irq, 0);
>>> Since you are now targeting random irqchips (as opposed to a GIC
>>> specifically), what guarantees that the timer is a per-cpu IRQ?
>> This is the host interrupt controller - and we're already using percpu
>> irqs on it :). Also as it happens the RPi has them percpu (anything else
>> wouldn't make sense...).
> Not really. The RPi is faking percpu interrupts just to have some level
> of compatibility with the host arch timer driver. But nonetheless, if
> you're opening the code to something else than a GIC, then you should
> check that the interrupt you're getting is percpu.
This should already be covered by request_percpu_irq() in
kvm_timer_hyp_init(), no?
Alex
^ permalink raw reply
* [PATCH v4 2/2] KVM: arm/arm64: Route vtimer events to user space
From: Marc Zyngier @ 2016-09-22 12:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57E3CF5A.3050608@suse.de>
On 22/09/16 13:32, Alexander Graf wrote:
> On 09/20/2016 11:21 AM, Marc Zyngier wrote:
>> On 19/09/16 18:39, Alexander Graf wrote:
>>>
>>> On 19.09.16 16:48, Marc Zyngier wrote:
>>>
>>>>> +
>>>>> + /*
>>>>> + * So we can just explicitly mask or unmask the IRQ, gaining
>>>>> + * more compatibility with oddball irq controllers.
>>>>> + */
>>>>> + if (phys_active)
>>>>> + disable_percpu_irq(host_vtimer_irq);
>>>>> + else
>>>>> + enable_percpu_irq(host_vtimer_irq, 0);
>>>> Since you are now targeting random irqchips (as opposed to a GIC
>>>> specifically), what guarantees that the timer is a per-cpu IRQ?
>>> This is the host interrupt controller - and we're already using percpu
>>> irqs on it :). Also as it happens the RPi has them percpu (anything else
>>> wouldn't make sense...).
>> Not really. The RPi is faking percpu interrupts just to have some level
>> of compatibility with the host arch timer driver. But nonetheless, if
>> you're opening the code to something else than a GIC, then you should
>> check that the interrupt you're getting is percpu.
>
> This should already be covered by request_percpu_irq() in
> kvm_timer_hyp_init(), no?
Ah, true. Ignore me, then.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case
From: Lorenzo Pieralisi @ 2016-09-22 12:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E1F8815A9@lhreml507-mbx>
On Thu, Sep 22, 2016 at 11:10:13AM +0000, Gabriele Paoloni wrote:
> Hi Lorenzo, Bjorn
>
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> > Sent: 22 September 2016 10:50
> > To: Bjorn Helgaas
> > Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin
> > Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan Kaya;
> > Jayachandran C; Christopher Covington; Duc Dang; Robert Richter; Marcin
> > Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux-
> > pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Linaro ACPI
> > Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong
> > (C); Gabriele Paoloni; Jeff Hugo; linux-acpi at vger.kernel.org; linux-
> > kernel at vger.kernel.org; Rafael J. Wysocki
> > Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-
> > specific register range for ACPI case
> >
> > On Wed, Sep 21, 2016 at 01:04:57PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
> > > > On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
> > > > > On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
> > > >
> > > > [...]
> > > >
> > > > > > None of these platforms can be fixed entirely in software, and
> > given
> > > > > > that we will not be adding quirks for new broken hardware, we
> > should
> > > > > > ask ourselves whether having two versions of a quirk, i.e., one
> > for
> > > > > > broken hardware + currently shipping firmware, and one for the
> > same
> > > > > > broken hardware with fixed firmware is really an improvement
> > over what
> > > > > > has been proposed here.
> > > > >
> > > > > We're talking about two completely different types of quirks:
> > > > >
> > > > > 1) MCFG quirks to use memory-mapped config space that doesn't
> > quite
> > > > > conform to the ECAM model in the PCIe spec, and
> > > > >
> > > > > 2) Some yet-to-be-determined method to describe address space
> > > > > consumed by a bridge.
> > > > >
> > > > > The first two patches of this series are a nice implementation
> > for 1).
> > > > > The third patch (ThunderX-specific) is one possibility for 2),
> > but I
> > > > > don't like it because there's no way for generic software like
> > the
> > > > > ACPI core to discover these resources.
> > > >
> > > > Ok, so basically this means that to implement (2) we need to assign
> > > > some sort of _HID to these quirky PCI bridges (so that we know what
> > > > device they represent and we can retrieve their _CRS). I take from
> > > > this discussion that the goal is to make sure that all non-config
> > > > resources have to be declared through _CRS device objects, which is
> > > > fine but that requires a FW update (unless we can fabricate ACPI
> > > > devices and corresponding _CRS in the kernel whenever we match a
> > > > given MCFG table signature).
> > >
> > > All resources consumed by ACPI devices should be declared through
> > > _CRS. If you want to fabricate ACPI devices or _CRS via kernel
> > > quirks, that's fine with me. This could be triggered via MCFG
> > > signature, DMI info, host bridge _HID, etc.
> >
> > I think the PNP quirk approach + PNP0c02 resource put forward by Gab
> > is enough.
>
> Great thanks as we take a final decision I will ask Dogndgong to submit
> another RFC based on this approach
>
> >
> > > > We discussed this already and I think we should make a decision:
> > > >
> > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-
> > March/414722.html
> > > >
> > > > > > > I'd like to step back and come up with some understanding of
> > how
> > > > > > > non-broken firmware *should* deal with this issue. Then, if
> > we *do*
> > > > > > > work around this particular broken firmware in the kernel, it
> > would be
> > > > > > > nice to do it in a way that fits in with that understanding.
> > > > > > >
> > > > > > > For example, if a companion ACPI device is the preferred
> > solution, an
> > > > > > > ACPI quirk could fabricate a device with the required
> > resources. That
> > > > > > > would address the problem closer to the source and make it
> > more likely
> > > > > > > that the rest of the system will work correctly: /proc/iomem
> > could
> > > > > > > make sense, things that look at _CRS generically would work
> > (e.g,
> > > > > > > /sys/, an admittedly hypothetical "lsacpi", etc.)
> > > > > > >
> > > > > > > Hard-coding stuff in drivers is a point solution that doesn't
> > provide
> > > > > > > any guidance for future platforms and makes it likely that
> > the hack
> > > > > > > will get copied into even more drivers.
> > > > > > >
> > > > > >
> > > > > > OK, I see. But the guidance for future platforms should be 'do
> > not
> > > > > > rely on quirks', and what I am arguing here is that the more we
> > polish
> > > > > > up this code and make it clean and reusable, the more likely it
> > is
> > > > > > that will end up getting abused by new broken hardware that we
> > set out
> > > > > > to reject entirely in the first place.
> > > > > >
> > > > > > So of course, if the quirk involves claiming resources, let's
> > make
> > > > > > sure that this occurs in the cleanest and most compliant way
> > possible.
> > > > > > But any factoring/reuse concerns other than for the current
> > crop of
> > > > > > broken hardware should be avoided imo.
> > > > >
> > > > > If future hardware is completely ECAM-compliant and we don't need
> > any
> > > > > more MCFG quirks, that would be great.
> > > >
> > > > Yes.
> > > >
> > > > > But we'll still need to describe that memory-mapped config space
> > > > > somewhere. If that's done with PNP0C02 or similar devices (as is
> > done
> > > > > on my x86 laptop), we'd be all set.
> > > >
> > > > I am not sure I understand what you mean here. Are you referring
> > > > to MCFG regions reported as PNP0c02 resources through its _CRS ?
> > >
> > > Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges
> > > reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02
> > > device.
> >
> > Ok, that's agreed. It goes without saying that since you are quoting
> > the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device
> > _CRS I will consider that a FW bug.
> >
> > > > IIUC PNP0C02 is a reservation mechanism, but it does not help us
> > > > associate its _CRS to a specific PCI host bridge instance, right ?
> > >
> > > Gab proposed a hierarchy that *would* associate a PNP0C02 device with
> > > a PCI bridge:
> > >
> > > Device (PCI1) {
> > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > Method (_CRS, 0, Serialized) { // Root complex resources
> > (windows) }
> > > Device (RES0) {
> > > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > > Name (_CRS, ResourceTemplate () { ... }
> > > }
> > > }
> > >
> > > That's a possibility. The PCI Firmware Spec suggests putting RES0 at
> > > the root (under \_SB), but I don't know why.
> > >
> > > Putting it at the root means we couldn't generically associate it
> > with
> > > a bridge, although I could imagine something like this:
> > >
> > > Device (RES1) {
> > > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > > Name (_CRS, ResourceTemplate () { ... }
> > > Method (BRDG) { "PCI1" } // hand-wavy ASL
> > > }
> > > Device (PCI1) {
> > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > Method (_CRS, 0, Serialized) { // Root complex resources
> > (windows) }
> > > }
> > >
> > > Where you could search PNP0C02 devices for a cookie that matched the
> > > host bridge.o
> >
> > Ok, I am fine with both and I think we are converging, but the way
> > to solve this problem has to be uniform for all ARM partners (and
> > not only ARM). Two points here:
> >
> > 1) Adding a device/subdevice allows people to add a _CRS reporting the
> > non-window bridge resources. Fine. It also allows people to chuck in
> > there all sorts of _DSD properties to describe their PCI host bridge
> > as it is done with DT properties (those _DSD can contain eg clocks
> > etc.), this may be tempting (so that they can reuse the same DT
> > driver and do not have to update their firmware) but I want to be
> > clear here: that must not happen. So, a subdevice with a _CRS to
> > report resources, yes, but it will stop there.
> > 2) It is unclear to me how to formalize the above. People should not
> > write FW by reading the PCI mailing list, so these guidelines have
> > to
> > be written, somehow. I do not want to standardize quirks, I want
> > to prevent random ACPI table content, which is different.
> > Should I report this to the ACPI spec working group ? If we do
> > not do that everyone will go solve this problem as they deem fit.
> >
>
> Do we really need to formalize this?
>
> As we discussed in the Linaro call at the moment we have few vendors
> that need quirks and we want to avoid promoting/accepting quirks for
> the future.
>
> At the time of the call I think we decided to informally accept a set
> of quirks for the current platforms and reject any other quirk coming
> after a certain date/kernel version (this to be decided).
>
> I am not sure if there is a way to document/formalize a temporary
> exception from the rule...
- (1) will be enforced.
- We do not know whether PNP0c02 can be used in non-root devices _CRS
- Are we sure (given that we are implementing this to make sure we are
able to validate resources) that it is valid to have a subdevice with
a _CRS whose resources are not contained in its parent _CRS address
space (because that's exactly the case for these quirks) ?
That's what I mean by formalizing, I want to know how PNP0c02 should
be used. We all want platforms with quirks to be enabled asap but only
if we stick to the ACPI specifications. On top of that, with the
bindings above, the kernel would end up creating a platform device for
the "fake" device with a _CRS approach, which is questionable.
Lorenzo
>
> Thanks
>
> Gab
>
>
> > [...]
> >
> > > > For FW that is immutable I really do not see what we can do apart
> > > > from hardcoding the non-config resources (consumed by a bridge),
> > > > somehow.
> > >
> > > Right. Well, I assume you mean we should hard-code "non-window
> > > resources consumed directly by a bridge". If firmware in the field
> > is
> > > broken, we should work around it, and that may mean hard-coding some
> > > resources.
> > >
> > > My point is that the hard-coding should not be buried in a driver
> > > where it's invisible to the rest of the kernel. If we hard-code it
> > in
> > > a quirk that adds _CRS entries, then the kernel will work just like
> > it
> > > would if the firmware had been correct in the first place. The
> > > resource will appear in /sys/devices/pnp*/*/resources and
> > /proc/iomem,
> > > and if we ever used _SRS to assign or move ACPI devices, we would
> > know
> > > to avoid the bridge resource.
> >
> > We are in complete agreement here.
> >
> > Thanks,
> > Lorenzo
>
^ permalink raw reply
* [PATCH v5] KVM: arm/arm64: Route vtimer events to user space
From: Alexander Graf @ 2016-09-22 12:52 UTC (permalink / raw)
To: linux-arm-kernel
We have 2 modes for dealing with interrupts in the ARM world. We can either
handle them all using hardware acceleration through the vgic or we can emulate
a gic in user space and only drive CPU IRQ pins from there.
Unfortunately, when driving IRQs from user space, we never tell user space
about timer events that may result in interrupt line state changes, so we
lose out on timer events if we run with user space gic emulation.
This patch fixes that by syncing user space's view of the vtimer irq line
with the kvm view of that same line.
With this patch I can successfully run edk2 and Linux with user space gic
emulation.
Signed-off-by: Alexander Graf <agraf@suse.de>
---
v1 -> v2:
- Add back curly brace that got lost
v2 -> v3:
- Split into patch set
v3 -> v4:
- Improve documentation
v4 -> v5:
- Rewrite to use pending state sync in sregs (marc)
- Remove redundant checks of vgic_initialized()
- qemu tree to try this out: https://github.com/agraf/u-boot.git no-kvm-irqchip-for-v5
---
Documentation/virtual/kvm/api.txt | 26 ++++++++
arch/arm/include/uapi/asm/kvm.h | 3 +
arch/arm/kvm/arm.c | 14 ++---
arch/arm64/include/uapi/asm/kvm.h | 3 +
include/kvm/arm_arch_timer.h | 2 +-
include/uapi/linux/kvm.h | 6 ++
virt/kvm/arm/arch_timer.c | 129 ++++++++++++++++++++++++++------------
7 files changed, 134 insertions(+), 49 deletions(-)
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 739db9a..8049327 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3928,3 +3928,29 @@ In order to use SynIC, it has to be activated by setting this
capability via KVM_ENABLE_CAP ioctl on the vcpu fd. Note that this
will disable the use of APIC hardware virtualization even if supported
by the CPU, as it's incompatible with SynIC auto-EOI behavior.
+
+8.3 KVM_CAP_ARM_TIMER
+
+Architectures: arm, arm64
+This capability, if KVM_CHECK_EXTENSION indicates that it is available and no
+in-kernel interrupt controller is in use, means that that the kernel populates
+the vcpu's run->s.regs.kernel_timer_pending field with timers that are currently
+considered pending by kvm.
+
+If active, it also allows user space to propagate its own pending state of timer
+interrupt lines using run->s.regs.user_timer_pending. If those two fields
+mismatch during CPU execution, kvm will exit to user space to give it a chance
+to update its own interrupt pending status. This usually involves triggering
+an interrupt line on a user space emulated interrupt controller.
+
+The fields run->s.regs.kernel_timer_pending and run->s.regs.user_timer_pending
+are available independent of run->kvm_valid_regs or run->kvm_dirty_regs bits.
+If no in-kernel interrupt controller is used and the capability exists, they
+will always be available and used.
+
+Currently the following bits are defined for both bitmaps:
+
+ KVM_ARM_TIMER_VTIMER - virtual timer
+
+Future versions of kvm may implement additional timer events. These will get
+indicated by additional KVM_CAP extensions.
diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index a2b3eb3..caad81d 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -105,6 +105,9 @@ struct kvm_debug_exit_arch {
};
struct kvm_sync_regs {
+ /* Used with KVM_CAP_ARM_TIMER */
+ u8 kernel_timer_pending;
+ u8 user_timer_pending;
};
struct kvm_arch_memory_slot {
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 75f130e..dc19221 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -187,6 +187,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
case KVM_CAP_ARM_PSCI_0_2:
case KVM_CAP_READONLY_MEM:
case KVM_CAP_MP_STATE:
+ case KVM_CAP_ARM_TIMER:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
@@ -474,13 +475,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
return ret;
}
- /*
- * Enable the arch timers only if we have an in-kernel VGIC
- * and it has been properly initialized, since we cannot handle
- * interrupts from the virtual timer with a userspace gic.
- */
- if (irqchip_in_kernel(kvm) && vgic_initialized(kvm))
- ret = kvm_timer_enable(vcpu);
+ ret = kvm_timer_enable(vcpu);
return ret;
}
@@ -588,7 +583,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
*/
preempt_disable();
kvm_pmu_flush_hwstate(vcpu);
- kvm_timer_flush_hwstate(vcpu);
+ if (kvm_timer_flush_hwstate(vcpu)) {
+ ret = -EINTR;
+ run->exit_reason = KVM_EXIT_INTR;
+ }
kvm_vgic_flush_hwstate(vcpu);
local_irq_disable();
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 3051f86..9aac860 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -143,6 +143,9 @@ struct kvm_debug_exit_arch {
#define KVM_GUESTDBG_USE_HW (1 << 17)
struct kvm_sync_regs {
+ /* Used with KVM_CAP_ARM_TIMER */
+ u8 kernel_timer_pending;
+ u8 user_timer_pending;
};
struct kvm_arch_memory_slot {
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index dda39d8..8cd7240 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -63,7 +63,7 @@ void kvm_timer_init(struct kvm *kvm);
int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
const struct kvm_irq_level *irq);
void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
-void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
+int kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 300ef25..1fc02d7 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -870,6 +870,7 @@ struct kvm_ppc_smmu_info {
#define KVM_CAP_S390_USER_INSTR0 130
#define KVM_CAP_MSI_DEVID 131
#define KVM_CAP_PPC_HTM 132
+#define KVM_CAP_ARM_TIMER 133
#ifdef KVM_CAP_IRQ_ROUTING
@@ -1327,4 +1328,9 @@ struct kvm_assigned_msix_entry {
#define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0)
#define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1)
+/* Available with KVM_CAP_ARM_TIMER */
+
+/* Bits for run->s.regs.{user,kernel}_timer_pending */
+#define KVM_ARM_TIMER_VTIMER (1 << 0)
+
#endif /* __LINUX_KVM_H */
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 4309b60..0c6fc38 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -166,21 +166,36 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
return cval <= now;
}
+/*
+ * Synchronize the timer IRQ state with the interrupt controller.
+ */
static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
{
int ret;
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
- BUG_ON(!vgic_initialized(vcpu->kvm));
-
timer->active_cleared_last = false;
timer->irq.level = new_level;
- trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
+ trace_kvm_timer_update_irq(vcpu->vcpu_id, host_vtimer_irq,
timer->irq.level);
- ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
- timer->irq.irq,
- timer->irq.level);
- WARN_ON(ret);
+
+ if (irqchip_in_kernel(vcpu->kvm)) {
+ BUG_ON(!vgic_initialized(vcpu->kvm));
+
+ /* Fire the timer in the VGIC */
+ ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
+ timer->irq.irq,
+ timer->irq.level);
+
+ WARN_ON(ret);
+ } else {
+ struct kvm_sync_regs *regs = &vcpu->run->s.regs;
+
+ /* Populate the timer bitmap for user space */
+ regs->kernel_timer_pending &= ~KVM_ARM_TIMER_VTIMER;
+ if (new_level)
+ regs->kernel_timer_pending |= KVM_ARM_TIMER_VTIMER;
+ }
}
/*
@@ -197,7 +212,8 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
* because the guest would never see the interrupt. Instead wait
* until we call this function from kvm_timer_flush_hwstate.
*/
- if (!vgic_initialized(vcpu->kvm) || !timer->enabled)
+ if ((irqchip_in_kernel(vcpu->kvm) && !vgic_initialized(vcpu->kvm)) ||
+ !timer->enabled)
return -ENODEV;
if (kvm_timer_should_fire(vcpu) != timer->irq.level)
@@ -248,15 +264,20 @@ void kvm_timer_unschedule(struct kvm_vcpu *vcpu)
*
* Check if the virtual timer has expired while we were running in the host,
* and inject an interrupt if that was the case.
+ *
+ * Returns:
+ *
+ * 0 - success
+ * 1 - need exit to user space
*/
-void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
+int kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
{
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
bool phys_active;
int ret;
if (kvm_timer_update_state(vcpu))
- return;
+ return 0;
/*
* If we enter the guest with the virtual input level to the VGIC
@@ -275,38 +296,61 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
* to ensure that hardware interrupts from the timer triggers a guest
* exit.
*/
- phys_active = timer->irq.level ||
- kvm_vgic_map_is_active(vcpu, timer->irq.irq);
-
- /*
- * We want to avoid hitting the (re)distributor as much as
- * possible, as this is a potentially expensive MMIO access
- * (not to mention locks in the irq layer), and a solution for
- * this is to cache the "active" state in memory.
- *
- * Things to consider: we cannot cache an "active set" state,
- * because the HW can change this behind our back (it becomes
- * "clear" in the HW). We must then restrict the caching to
- * the "clear" state.
- *
- * The cache is invalidated on:
- * - vcpu put, indicating that the HW cannot be trusted to be
- * in a sane state on the next vcpu load,
- * - any change in the interrupt state
- *
- * Usage conditions:
- * - cached value is "active clear"
- * - value to be programmed is "active clear"
- */
- if (timer->active_cleared_last && !phys_active)
- return;
-
- ret = irq_set_irqchip_state(host_vtimer_irq,
- IRQCHIP_STATE_ACTIVE,
- phys_active);
- WARN_ON(ret);
+ if (irqchip_in_kernel(vcpu->kvm)) {
+ phys_active = timer->irq.level ||
+ kvm_vgic_map_is_active(vcpu, timer->irq.irq);
+
+ /*
+ * We want to avoid hitting the (re)distributor as much as
+ * possible, as this is a potentially expensive MMIO access
+ * (not to mention locks in the irq layer), and a solution for
+ * this is to cache the "active" state in memory.
+ *
+ * Things to consider: we cannot cache an "active set" state,
+ * because the HW can change this behind our back (it becomes
+ * "clear" in the HW). We must then restrict the caching to
+ * the "clear" state.
+ *
+ * The cache is invalidated on:
+ * - vcpu put, indicating that the HW cannot be trusted to be
+ * in a sane state on the next vcpu load,
+ * - any change in the interrupt state
+ *
+ * Usage conditions:
+ * - cached value is "active clear"
+ * - value to be programmed is "active clear"
+ */
+ if (timer->active_cleared_last && !phys_active)
+ return 0;
+
+ ret = irq_set_irqchip_state(host_vtimer_irq,
+ IRQCHIP_STATE_ACTIVE,
+ phys_active);
+ WARN_ON(ret);
+ } else {
+ struct kvm_sync_regs *regs = &vcpu->run->s.regs;
+
+ /*
+ * User space handles timer events, so we need to check whether
+ * its view of the world is in sync with ours.
+ */
+ if (regs->kernel_timer_pending != regs->user_timer_pending) {
+ /* Return to user space */
+ return 1;
+ }
+
+ /*
+ * As long as user space is aware that the timer is pending,
+ * we do not need to get new host timer events.
+ */
+ if (timer->irq.level)
+ disable_percpu_irq(host_vtimer_irq);
+ else
+ enable_percpu_irq(host_vtimer_irq, 0);
+ }
timer->active_cleared_last = !phys_active;
+ return 0;
}
/**
@@ -479,6 +523,10 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
if (timer->enabled)
return 0;
+ /* No need to route physical IRQs when we don't use the vgic */
+ if (!irqchip_in_kernel(vcpu->kvm))
+ goto no_vgic;
+
/*
* Find the physical IRQ number corresponding to the host_vtimer_irq
*/
@@ -502,6 +550,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
if (ret)
return ret;
+no_vgic:
/*
* There is a potential race here between VCPUs starting for the first
--
1.8.5.6
^ permalink raw reply related
* [RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore
From: Mark Rutland @ 2016-09-22 12:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474543806-19210-1-git-send-email-ard.biesheuvel@linaro.org>
Hi Ard,
On Thu, Sep 22, 2016 at 12:30:03PM +0100, Ard Biesheuvel wrote:
> ================================================================================
> NOTE: this is a work in progress, and not fully functional yet. In particular,
> the actual MMC host protocol methods are stubbed out at the moment, and need to
> be wired up to the Linux device drivers.
> ================================================================================
>
> On mobile and embedded systems, there is usually only a single MMC device for
> non-volatile storage, which sits behind a controller that is owned by the OS at
> runtime. This makes it difficult to host the UEFI variable store on MMC as well,
> since the UEFI runtime services routines expect ownership of the underlying
> device as well.
>
> This series proposes an approach to work around this. It implements the UEFI
> MMC host protocol in the kernel, in a way that makes it possible to expose it
> to the firmware. At the same time, the firmware needs be set up for this, i.e.,
> it needs to expose its MMC host protocol pointer via a UEFI configuration table,
> so that the kernel can override it if it decides to expose this functionality
> to the firmware.
At a high level, and assuming a number of details from previous
discussions, I think the general approach of having the kernel mediate
access to the MMC makes sense.
However, I don't think that this series has enough detail for critical
review, even at the interface level. e.g. there is no mention of how
this caters for replay attacks and so on (which the current spec
sidesteps). I'm under the impression that there are mechanisms which
have been discussed for this, and I hope this is simply an oversight.
I also think that this needs to go via the USWG (and to the UEFI spec),
before we can consider using it. I say this because:
* This is critical to correct operation of variable storage as required
for the standard boot flow. This is a major change to the way variable
storage works today, with a number of (security) implications.
* There are others in this space trying to use the same class of
hardware, e.g. FreeBSD. We don't want a Linux-specific interface, nor
do we want a proliferation of interfaces for this purpose.
I have a few other general concerns:
* Identification of the relevant MMC device(s).
Patch 3 suggests a devicetree property. I don't think that the
'linux,' prefix makes sense, and it's not clear what we would do with
ACPI.
Does the MMC device not have some identifier we can query, so that we
can match this up without requiring additional info in ACPI/DT?
* Lifetime guarantees
When is it valid for EFI to call the MMC proxy? Can other services
(e.g. ACPI) call this?
How do we handle kexec/kdump? e.g. how do we teardown the interface
before branching to a new kernel, how do we safely tear down a crashed
kernel's interface, what can we call before doing so?
How do we handle suspend/resume? e.g. is it necessary to re-register
upon resume?
Thanks,
Mark.
> Note that these patches are based on patches in the EFI tree that are queued
> for v4.9, which replace the runtime wrappers spinlock with a semaphore. This
> allows us to sleep in the firmware callbacks.
>
> Prerequisites for using these patches:
>
> * qemu-system-aarch64 built from this branch:
> https://git.linaro.org/people/ard.biesheuvel/qemu.git/shortlog/refs/heads/mach-virt-pl181
> which adds a PL181 SD/MMC controller to the mach-virt model, and exposes it
> via the device tree. It also sets the 'linux,uefi-varstore' property on this
> node.
>
> * UEFI firmware built from this branch:
> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/mmc-proxy
>
> Build using the following command
> build -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemuMmcVars.dsc
>
> Run using
> qemu-system-aarch64 \
> -M virt -cpu cortex-a57 -m 2048 \
> -device virtio-blk-device,drive=boot \
> -drive if=none,id=boot,file=fat:<path-of-Image>,format=raw \
> -kernel <edk2-dir>/Build/ArmVirtQemuMmcVars-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd \
> -sd <edk2-dir>/build/edk2/Build/ArmVirtQemuMmcVars-AARCH64/DEBUG_GCC5/FV/QEMU_VARS.fd \
> -nographic $@
>
> This will give you an UEFI environment which keeps its UEFI variables in the
> emulated MMC volume, and exposes its MMC host protocol in a way that allows
> these patches to hook into it.
>
> Patches #1 and #2 implement the arch specific hooks to preserve/restore the NEON
> registers that the firmware may expect to be preserved across function calls.
>
> Patch #3 implements the plumbing to call back into the kernel from the firmware.
>
> Please comment on whether this approach seems feasible, and in particular, how
> on earth I should wire this up to the actual MMC code.
>
> Thanks,
> Ard.
>
> Ard Biesheuvel (3):
> efi/arm64: add SIMD stash/unstash operations
> efi/arm: add SIMD stash/unstash operations
> efi: implement MMC proxy support for the UEFI variable store
>
> arch/arm/include/asm/efi.h | 11 +
> arch/arm64/include/asm/efi.h | 33 +++
> drivers/firmware/efi/Kconfig | 9 +
> drivers/firmware/efi/Makefile | 1 +
> drivers/firmware/efi/arm-init.c | 2 +
> drivers/firmware/efi/mmc-proxy.c | 222 ++++++++++++++++++++
> include/linux/efi.h | 1 +
> 7 files changed, 279 insertions(+)
> create mode 100644 drivers/firmware/efi/mmc-proxy.c
>
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH 0/2] BQL support and fix for a regression issue
From: Eric Dumazet @ 2016-09-22 13:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160922.082540.992436214283504080.davem@davemloft.net>
On Thu, 2016-09-22 at 08:25 -0400, David Miller wrote:
> From: sunil.kovvuri at gmail.com
> Date: Thu, 22 Sep 2016 14:35:19 +0530
>
> > From: Sunil Goutham <sgoutham@cavium.com>
> >
> > These patches add byte queue limit support and also fixes a regression
> > issue introduced by commit
> > 'net: thunderx: Use netdev's name for naming VF's interrupts'
>
> The correct way to refer to commits when you fix a bug is
> to use the "Fixes: " tag right before the signoffs and ACKs.
>
> The correct form is:
>
> Fixes: $SHA1_ID ("Commit header line.")
>
> You'll see this in many other netdev commits.
Thanks a lot David for this enforcement, it really helps bug tracking
and backports
Note that the $SHA1_ID should be truncated to 12 first chars.
Refer to Documentation/SubmittingPatches around line 191 for more
details.
^ permalink raw reply
* [PATCHv2 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts
From: Uwe Kleine-König @ 2016-09-22 13:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160912094733.21501-2-richard.genoud@gmail.com>
On Mon, Sep 12, 2016 at 11:47:31AM +0200, Richard Genoud wrote:
> This function returns true if CTS and RTS are used as GPIOs.
> Some drivers (like atmel_serial) needs to know if the flow control is
> handled by the controller or by GPIOs.
>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
> drivers/tty/serial/serial_mctrl_gpio.c | 8 ++++++++
> drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> index d2da6aa7f27d..93bed8c99796 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> @@ -17,6 +17,7 @@
> #include <linux/err.h>
> #include <linux/device.h>
> #include <linux/irq.h>
> +#include <linux/err.h>
> #include <linux/gpio/consumer.h>
> #include <linux/termios.h>
> #include <linux/serial_core.h>
> @@ -72,6 +73,13 @@ struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
> }
> EXPORT_SYMBOL_GPL(mctrl_gpio_to_gpiod);
>
> +bool mctrl_gpio_use_rtscts(struct mctrl_gpios *gpios)
> +{
> + return !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS)) &&
> + !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_RTS));
mctrl_gpio_to_gpiod cannot return an error pointer. So this is
(surprise!) a wrong usage of IS_ERR_OR_NULL.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* [PATCHv2 2/3] tty/serial: at91: fix hardware handshake with GPIOs
From: Uwe Kleine-König @ 2016-09-22 13:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160912094733.21501-3-richard.genoud@gmail.com>
On Mon, Sep 12, 2016 at 11:47:32AM +0200, Richard Genoud wrote:
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when GPIOs
> where used.
s/where/were/
> Hardware handshake with GPIOs used to work before this commit because
> the CRTSCTS flag (termios->c_cflag) was set, but not the
> ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware handshake
> enabled, but not handled by the controller.
>
> This commit restores this behaviour.
>
> NB: -stable is not Cced because it doesn't cleanly apply on 4.1+
> and it will also need previous commit:
> "serial: mctrl_gpio: implement mctrl_gpio_use_rtscts"
>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> Fixes: 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled")
> ---
> drivers/tty/serial/atmel_serial.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 2eaa18ddef61..e9b4fbf88c2d 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -2025,6 +2025,7 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
> static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> struct ktermios *old)
> {
> + struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> unsigned long flags;
> unsigned int old_mode, mode, imr, quot, baud;
>
> @@ -2126,8 +2127,12 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> atmel_uart_writel(port, ATMEL_US_TTGR,
> port->rs485.delay_rts_after_send);
> mode |= ATMEL_US_USMODE_RS485;
> - } else if (termios->c_cflag & CRTSCTS) {
> - /* RS232 with hardware handshake (RTS/CTS) */
> + } else if ((termios->c_cflag & CRTSCTS) &&
> + !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
IMHO the behaviour of the hw controlled pins shouldn't change when
mctrl_gpio is in use (if possible). But I don't understand the issue so
I guess you need a better changelog.
> + /*
> + * RS232 with hardware handshake (RTS/CTS)
> + * handled by the controller.
> + */
> if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) {
> dev_info(port->dev, "not enabling hardware flow control because DMA is used");
> termios->c_cflag &= ~CRTSCTS;
> @@ -2135,7 +2140,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> mode |= ATMEL_US_USMODE_HWHS;
> }
> } else {
> - /* RS232 without hadware handshake */
> + /* RS232 without hadware handshake or controlled by GPIOs */
When touching this line, please also do s/hadware/hardware/.
> mode |= ATMEL_US_USMODE_NORMAL;
> }
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* [PATCH v2] arm: dts: zynq: Add MicroZed board support
From: Jagan Teki @ 2016-09-22 13:21 UTC (permalink / raw)
To: linux-arm-kernel
From: Jagan Teki <jteki@openedev.com>
Added basic dts support for MicroZed board.
- UART
- SDHCI
- Ethernet
Cc: Soren Brinkmann <soren.brinkmann@xilinx.com>
Cc: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Jagan Teki <jteki@openedev.com>
---
Changes for v2:
- Add SDHCI
- Add Ethernet
arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/zynq-microzed.dts | 95 +++++++++++++++++++++++++++++++++++++
2 files changed, 96 insertions(+)
create mode 100644 arch/arm/boot/dts/zynq-microzed.dts
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index faacd52..4d7b858 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -862,6 +862,7 @@ dtb-$(CONFIG_ARCH_VT8500) += \
wm8750-apc8750.dtb \
wm8850-w70v2.dtb
dtb-$(CONFIG_ARCH_ZYNQ) += \
+ zynq-microzed.dtb \
zynq-parallella.dtb \
zynq-zc702.dtb \
zynq-zc706.dtb \
diff --git a/arch/arm/boot/dts/zynq-microzed.dts b/arch/arm/boot/dts/zynq-microzed.dts
new file mode 100644
index 0000000..9e64496
--- /dev/null
+++ b/arch/arm/boot/dts/zynq-microzed.dts
@@ -0,0 +1,95 @@
+/*
+ * Copyright (C) 2015 Jagan Teki <jteki@openedev.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ */
+/dts-v1/;
+/include/ "zynq-7000.dtsi"
+
+/ {
+ model = "Zynq MicroZED Development Board";
+ compatible = "xlnx,zynq-microzed", "xlnx,zynq-7000";
+
+ aliases {
+ ethernet0 = &gem0;
+ serial0 = &uart1;
+ };
+
+ memory {
+ device_type = "memory";
+ reg = <0x0 0x40000000>;
+ };
+
+ chosen {
+ bootargs = "earlycon";
+ stdout-path = "serial0:115200n8";
+ };
+
+ usb_phy0: phy0 {
+ compatible = "usb-nop-xceiv";
+ #phy-cells = <0>;
+ };
+};
+
+&clkc {
+ ps-clk-frequency = <33333333>;
+};
+
+&gem0 {
+ status = "okay";
+ phy-mode = "rgmii-id";
+ phy-handle = <ðernet_phy>;
+
+ ethernet_phy: ethernet-phy at 0 {
+ reg = <0>;
+ };
+};
+
+&sdhci0 {
+ status = "okay";
+};
+
+&uart1 {
+ status = "okay";
+};
+
+&usb0 {
+ status = "okay";
+ dr_mode = "host";
+ usb-phy = <&usb_phy0>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb0_default>;
+};
+
+&pinctrl0 {
+ pinctrl_usb0_default: usb0-default {
+ mux {
+ groups = "usb0_0_grp";
+ function = "usb0";
+ };
+
+ conf {
+ groups = "usb0_0_grp";
+ slew-rate = <0>;
+ io-standard = <1>;
+ };
+
+ conf-rx {
+ pins = "MIO29", "MIO31", "MIO36";
+ bias-high-impedance;
+ };
+
+ conf-tx {
+ pins = "MIO28", "MIO30", "MIO32", "MIO33", "MIO34",
+ "MIO35", "MIO37", "MIO38", "MIO39";
+ bias-disable;
+ };
+ };
+};
--
2.7.4
^ permalink raw reply related
* [PATCH v2] musb: Export musb_root_disconnect for use in modules
From: Bin Liu @ 2016-09-22 13:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160921205133.GC3981@uda0271908>
On Wed, Sep 21, 2016 at 03:51:33PM -0500, Bin Liu wrote:
> Hi,
>
> On Sat, Sep 17, 2016 at 12:08:10PM +0200, Hans de Goede wrote:
> > Export musb_root_disconnect for use in modules, so that musb glue
> > code build as module can use it.
> >
> > This fixes the buildbot errors for -next in arm64-allmodconfig
> > and arm-allmodconfig.
> >
> > Fixes: 7cba17ec9adc8cf ("musb: sunxi: Add support for platform_set_mode")
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Applied. Thanks.
Removed it from my tree, since Greg already picked it.
Regards,
-Bin.
^ permalink raw reply
* [RFC PATCH 00/11] pci: support for configurable PCI endpoint
From: Arnd Bergmann @ 2016-09-22 13:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57DA5CC1.6030502@ti.com>
On Thursday, September 15, 2016 2:03:05 PM CEST Kishon Vijay Abraham I wrote:
> On Wednesday 14 September 2016 06:55 PM, Arnd Bergmann wrote:
> > On Wednesday, September 14, 2016 10:41:56 AM CEST Kishon Vijay Abraham I wrote:
> > I've added the drivers/ntb maintainers to Cc, given that there is
> > a certain degree of overlap between your work and the existing
> > code, I think they should be part of the discussion.
> >
> >> Known Limitation:
> >> *) Does not support multi-function devices
> >
> > If I understand it right, this was a problem for USB and adding
> > it later made it somewhat inconsistent. Maybe we can at least
> > try to come up with an idea of how multi-function devices
> > could be handled even if we don't implement it until someone
> > actually needs it.
>
> Actually IMO multi-function device in PCI should be much simpler than it is for
> USB. In the case of USB, all the functions in a multi-function device will
> share the same *usb configuration* . (USB device can have multiple
> configuration but only one can be enabled at a time). A multi-function USB
> device will still have a single vendor-id/product-id/class... So I think a
> separate library (composite.c) in USB makes sense.
Ok, makes sense.
> But in the case of PCI, every function can be treated independently since all
> the functions have it's own 4KB configuration space. Each function can be
> configured independently. Each can have it's own vendor-id/product-id/class..
> I'm not sure if we'll need a separate library for PCI like we have for USB.
I think it depends on whether we want to add the software multi-function
support you mention.
> Now the restriction for not allowing multi-function device is because of the
> following structure definition.
>
> struct pci_epc {
> ..
> struct pci_epf *epf;
> ..
> };
>
> EPC has a single reference to EPF and it is used *only* to notify the function
> driver when the link is up. (If this can be changed to use notification
> mechanism, multi-function devices can be supported here)
>
> One more place where this restriction arises is in designware driver
>
> struct dw_pcie_ep {
> ..
> u8 bar_to_atu[6];
> ..
> };
>
> We use single ATU window to configure a BAR (in BAR). If there are multiple
> functions, then this should also be modified since each function has 6 BARs.
>
> This can be fixed without much effort unless some other issue props up.
Ok.
> >
> > Is your hardware able to make the PCIe endpoint look like
> > a device with multiple PCI functions, or would one have to
> > do this in software inside of a single PCI function if we
> > ever need it?
>
> The hardware I have doesn't support multiple PCI functions (like having a
> separate configuration space for each function). It has a dedicated space for
> configuration space supporting only one function. [Section 24.9.7.3.2
> PCIe_SS_EP_CFG_DBICS Register Description in [1]].
>
> yeah, it has to be done in software (but that won't be multi-function device in
> PCI terms).
>
> [1] -> http://www.ti.com/lit/ug/spruhz6g/spruhz6g.pdf
Ok, so in theory there can be other hardware (and quite likely is)
that supports multiple functions, and we can extend the framework
to support them without major obstacles, but your hardware doesn't,
so you kept it simple with one hardcoded function, right?
Seems completely reasonable to me.
> >> TODO:
> >> *) access buffers in RC
> >> *) raise MSI interrupts
> >> *) Enable user space control for the RC side PCI driver
> >
> > The user space control would end up just being one of several
> > gadget drivers, right? E.g. gadget drivers for standard hardware
> > (8250 uart, ATA, NVMe, some ethernet) could be done as kernel
> > drivers while a user space driver can be used for things that
> > are more unusual and that don't need to interface to another
> > part of the kernel?
>
> Actually I didn't mean that. It was more with respect to the host side PCI test
> driver (drivers/misc/pci_endpoint_test.c). Right now it validates BAR, irq
> itself. I wanted to change this so that the user controls which tests to run.
> (Like for USB gadget zero tests, testusb.c invokes ioctls to perform various
> tests). Similarly I want to have a userspace program invoke pci_endpoint_test
> to perform various PCI tests.
Ok, I see. So what I described above would be yet another function
driver that can be implemented, but so far, you have not planned
to do that because there was not need, right?
> >
> >> *) Adapt all other users of designware to use the new design (only
> >> dra7xx has been adapted)
> >
> > I don't fully understand this part. Does every designware based
> > driver need modifications, or are the changes to the
> > generic parts of the designware driver enough to make it
> > work for the simpler platforms?
>
> I have changed the core designware driver structures (like previously the
> platform drivers will only use pcie_port, but now I introduced struct dw_pcie
> to support both host and endpoint). This will break (compilation failure) all
> the designware based drivers (except dra7xx). All these drivers should be
> adapted to the new change (even if they work only in host mode these has to be
> adapted).
Ah, so we have to do two separate modifications to each designware driver:
a) make it work with your patch (mandatory)
b) make it support endpoint mode (optional)
> >> HOW TO:
> >>
> >> ON THE EP SIDE:
> >> ***************
> >>
> >> /* EP function is configured using configfs */
> >> # mount -t configfs none /sys/kernel/config
> >>
> >> /* PCI EP core layer creates "pci_ep" entry in configfs */
> >> # cd /sys/kernel/config/pci_ep/
> >>
> >> /*
> >> * This is the 1st step in creating an endpoint function. This
> >> * creates the endpoint function device *instance*. The string
> >> * before the .<num> suffix will identify the driver this
> >> * EP function will bind to.
> >> * Just pci_epf_test is also valid. The .<num> suffix is used
> >> * if there are multiple PCI controllers and all of them wants
> >> * to use the same function.
> >> */
> >> # mkdir pci_epf_test.0
> >
> > I haven't used USB gadgets, but I assume this is modeled around
> > the same interface. If there are notable differences, please mention
> > what they are. Otherwise the general concept seems rather nice to me.
>
> Yeah, both USB gadget and PCI endpoint use configfs interface but the semantics
> are quite different.
>
> Every directory in *usb_gadget* corresponds to a gadget device, and the gadget
> device has a functions sub-directory which has the USB functions. And these
> directories have fields or attributes specific to USB.
>
> But in the case of PCI, every directory in *pci_ep* corresponds to a PCI
> function and it has fields or attributes specific to PCI function.
Ok, I see.
> The main reason for using configfs for PCI endpoint is to give the users to
> control "which function has to be bound to which controller". The same concept
> is used for USB gadget as well but there it is "which gadget device has to be
> bound to which controller".
We should still find out whether it's important that you can have
a single PCI function with a software multi-function support of some
sort. We'd still be limited to six BARs in total, and would also need
something to identify those sub-functions, so implementing that might
get quite hairy.
Possibly this could be done at a higher level, e.g. by implementing
a PCI-virtio multiplexer that can host multiple virtio based devices
inside of a single PCI function. If we think that would be a good idea,
we should make sure the configfs interface is extensible enough to
handle that.
One use case I have in mind for this is to have a PCI function that
can use virtio to provide rootfs (virtio-blk or 9pfs), network
and console to the system that implements the PCI function (note
that this is the opposite direction of what almost everyone else
uses PCI devices for).
> > Let's talk (high-level) about the DT binding. I see that the way
> > you have done it here, one will need to have a different .dtb file
> > for a machine depending on whether the PCIe is used in host or
> > endpoint mode. The advantage of this way is that it's a much
> > cleaner binding (PCIe host bindings are a mess, and adding more
> > options to it will only make it worse), the downside is that
> > you can't decide at runtime what you want to use it for. E.g.
> > connecting two identical machines over PCIe requires deciding
> > in the bootloader which one is the endpoint, or using DT
> > overlays, which may be awkward for some users. Is this a realistic
> > use case, or do you expect that all machines will only ever be
> > used in one of the two ways?
>
> It would definitely be nice to select the mode at runtime. Even for this patch
> series, I added a temporary dtsi patch to configure the pci controller in EP
> mode (which can't be merged since the same controller is also used to test RC).
I think it should be possible to have two bindings that define a
distinct set of properties, and have one node that is marked
as "compatible" with both of them in order to let the OS choose
one or the other mode.
Arnd
^ permalink raw reply
* [RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore
From: Ard Biesheuvel @ 2016-09-22 13:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160922125854.GC26596@leverpostej>
(adding Charles whom I failed to cc)
On 22 September 2016 at 13:58, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Ard,
>
> On Thu, Sep 22, 2016 at 12:30:03PM +0100, Ard Biesheuvel wrote:
>> ================================================================================
>> NOTE: this is a work in progress, and not fully functional yet. In particular,
>> the actual MMC host protocol methods are stubbed out at the moment, and need to
>> be wired up to the Linux device drivers.
>> ================================================================================
>>
>> On mobile and embedded systems, there is usually only a single MMC device for
>> non-volatile storage, which sits behind a controller that is owned by the OS at
>> runtime. This makes it difficult to host the UEFI variable store on MMC as well,
>> since the UEFI runtime services routines expect ownership of the underlying
>> device as well.
>>
>> This series proposes an approach to work around this. It implements the UEFI
>> MMC host protocol in the kernel, in a way that makes it possible to expose it
>> to the firmware. At the same time, the firmware needs be set up for this, i.e.,
>> it needs to expose its MMC host protocol pointer via a UEFI configuration table,
>> so that the kernel can override it if it decides to expose this functionality
>> to the firmware.
>
> At a high level, and assuming a number of details from previous
> discussions, I think the general approach of having the kernel mediate
> access to the MMC makes sense.
>
OK.
> However, I don't think that this series has enough detail for critical
> review, even at the interface level. e.g. there is no mention of how
> this caters for replay attacks and so on (which the current spec
> sidesteps). I'm under the impression that there are mechanisms which
> have been discussed for this, and I hope this is simply an oversight.
>
This series does not take UEFI Secure Boot into account at all, and so
replay protection is not currently being addressed explicitly.
However, the reason for exposing an MMC host protocol rather than some
more abstract protocol (i.e., block I/O) is to allow firmware that
uses RPMB over MMC (e.g, with an authentication component residing in
the secure world) to reuse the same interface while maintaining the
same level of security it would have if it owned the MMC fully.
> I also think that this needs to go via the USWG (and to the UEFI spec),
> before we can consider using it. I say this because:
>
> * This is critical to correct operation of variable storage as required
> for the standard boot flow. This is a major change to the way variable
> storage works today, with a number of (security) implications.
>
The primary issue there is that the variable protocols are
architectural DXE protocols defined in the PI spec, not the UEFI spec
(and the MMC host protocol I am using here is not in any spec afaik),
and so implementing this in a UEFI compliant way is not currently
possible. Exposing a UEFI protocol like block I/O is not sufficient,
since the variable protocols are depended upon earlier in the boot
sequence (in UEFI implementations based on PI).
So yes, this needs to be discussed (and it will be, but not out in the
open, unfortunately.) However, it is unclear whether it is feasible to
address this at the UEFI level, or whether we will need to go beyond
and define something based on the PI spec directly. (/me looks at
Charles)
> * There are others in this space trying to use the same class of
> hardware, e.g. FreeBSD. We don't want a Linux-specific interface, nor
> do we want a proliferation of interfaces for this purpose.
>
No, we don't.
> I have a few other general concerns:
>
> * Identification of the relevant MMC device(s).
>
> Patch 3 suggests a devicetree property. I don't think that the
> 'linux,' prefix makes sense, and it's not clear what we would do with
> ACPI.
>
> Does the MMC device not have some identifier we can query, so that we
> can match this up without requiring additional info in ACPI/DT?
>
Good question. IIRC (but I'm a little rusty here), that is the case
for SD but not for MMC? (but it probably depends on the version) In
any case, there are other concerns here, regarding how the OS knows
which slice of the MMC is in use by the firmware. So the firmware
should expose sufficient information to the OS for it to figure out
which MMC volume it should expose to the firmware. UEFI generally uses
device paths for this, but I'm not sure if that is appropriate here.
> * Lifetime guarantees
>
> When is it valid for EFI to call the MMC proxy? Can other services
> (e.g. ACPI) call this?
>
> How do we handle kexec/kdump? e.g. how do we teardown the interface
> before branching to a new kernel, how do we safely tear down a crashed
> kernel's interface, what can we call before doing so?
>
> How do we handle suspend/resume? e.g. is it necessary to re-register
> upon resume?
>
All good questions, and exactly the kind of feedback I am seeking.
Tearing down the interface could be as simple as clearing the pointer,
but some synchronization is probably in order to make sure that no
calls are in progress.
But to clarify the purpose of this series: are there any concerns
regarding exposing callbacks to the firmware in general, and for MMC
access in particular, from the Linux side? This code seems to work as
expected, but I may have missed something important (and I still need
help to actually wire it up to the MMC code)
--
Ard.
^ permalink raw reply
* [PATCH v2] ASoC: rk3399_gru_sound: fix recording pop at first attempt
From: Xing Zheng @ 2016-09-22 13:50 UTC (permalink / raw)
To: linux-arm-kernel
From: Wonjoon Lee <woojoo.lee@samsung.com>
Pop happens when mclk applied but dmic's own boot-time
Specify dmic delay times in dt to make sure
clocks are ready earlier than dmic working
Signed-off-by: Wonjoon Lee <woojoo.lee@samsung.com>
Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
---
Changes in v2:
- rename dmic-delay to dmic-wakeup-delay-ms
.../bindings/sound/rockchip,rk3399-gru-sound.txt | 7 +++++++
sound/soc/rockchip/rk3399_gru_sound.c | 14 ++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/rockchip,rk3399-gru-sound.txt b/Documentation/devicetree/bindings/sound/rockchip,rk3399-gru-sound.txt
index f19b6c8..eac91db 100644
--- a/Documentation/devicetree/bindings/sound/rockchip,rk3399-gru-sound.txt
+++ b/Documentation/devicetree/bindings/sound/rockchip,rk3399-gru-sound.txt
@@ -6,10 +6,17 @@ Required properties:
connected to the codecs
- rockchip,codec: The phandle of the MAX98357A/RT5514/DA7219 codecs
+Optional properties:
+- dmic-wakeup-delay-ms : specify delay time (ms) for DMIC ready.
+ If this option is specified, which means it's required dmic need
+ delay for DMIC to ready so that rt5514 can avoid recording before
+ DMIC send valid data
+
Example:
sound {
compatible = "rockchip,rk3399-gru-sound";
rockchip,cpu = <&i2s0>;
rockchip,codec = <&max98357a &rt5514 &da7219>;
+ dmic-wakeup-delay-ms = <20>;
};
diff --git a/sound/soc/rockchip/rk3399_gru_sound.c b/sound/soc/rockchip/rk3399_gru_sound.c
index ee06489..9ed735a 100644
--- a/sound/soc/rockchip/rk3399_gru_sound.c
+++ b/sound/soc/rockchip/rk3399_gru_sound.c
@@ -38,6 +38,8 @@
#define SOUND_FS 256
+unsigned int rt5514_dmic_delay;
+
static struct snd_soc_jack rockchip_sound_jack;
static const struct snd_soc_dapm_widget rockchip_dapm_widgets[] = {
@@ -123,6 +125,9 @@ static int rockchip_sound_rt5514_hw_params(struct snd_pcm_substream *substream,
return ret;
}
+ /* Wait for DMIC stable */
+ msleep(rt5514_dmic_delay);
+
return 0;
}
@@ -343,6 +348,15 @@ static int rockchip_sound_probe(struct platform_device *pdev)
return -ENODEV;
}
+ /* Set DMIC delay */
+ ret = device_property_read_u32(&pdev->dev, "dmic-delay",
+ &rt5514_dmic_delay);
+ if (ret) {
+ rt5514_dmic_delay = 0;
+ dev_dbg(&pdev->dev,
+ "no optional property 'dmic-delay' found, default: no delay\n");
+ }
+
rockchip_dailinks[DAILINK_RT5514_DSP].cpu_name = kstrdup_const(dev_name(dev), GFP_KERNEL);
rockchip_dailinks[DAILINK_RT5514_DSP].cpu_dai_name = kstrdup_const(dev_name(dev), GFP_KERNEL);
rockchip_dailinks[DAILINK_RT5514_DSP].platform_name = kstrdup_const(dev_name(dev), GFP_KERNEL);
--
1.9.1
^ permalink raw reply related
* [PATCH] ASoC: rk3399_gru_sound: fix recording pop at first attempt
From: Xing Zheng @ 2016-09-22 13:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160919144404.GA18734@leverpostej>
Hi Mark,
On 2016?09?19? 22:44, Mark Rutland wrote:
> On Mon, Sep 19, 2016 at 10:29:39PM +0800, Xing Zheng wrote:
>> From: Wonjoon Lee <woojoo.lee@samsung.com>
>>
>> Pop happens when mclk applied but dmic's own boot-time
>> Specify dmic delay times in dt to make sure
>> clocks are ready earlier than dmic working
>>
>> Signed-off-by: Wonjoon Lee <woojoo.lee@samsung.com>
>> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
>> ---
>>
>> .../bindings/sound/rockchip,rk3399-gru-sound.txt | 6 ++++++
>> sound/soc/rockchip/rk3399_gru_sound.c | 14 ++++++++++++++
>> 2 files changed, 20 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/rockchip,rk3399-gru-sound.txt b/Documentation/devicetree/bindings/sound/rockchip,rk3399-gru-sound.txt
>> index f19b6c8..b7dd3ab 100644
>> --- a/Documentation/devicetree/bindings/sound/rockchip,rk3399-gru-sound.txt
>> +++ b/Documentation/devicetree/bindings/sound/rockchip,rk3399-gru-sound.txt
>> @@ -6,6 +6,12 @@ Required properties:
>> connected to the codecs
>> - rockchip,codec: The phandle of the MAX98357A/RT5514/DA7219 codecs
>>
>> +Optional properties:
>> +- dmic-delay : specify delay time for DMIC ready.
>> + If this option is specified, which means it's required dmic need
>> + delay for DMIC to ready so that rt5514 can avoid recording before
>> + DMIC send valid data
> What units is this in? Per the code it looks like ms, so if we follow
> Documentation/devicetree/bindings/property-units.txt, thous should be
> named something like dmic-enable-delay-ms.
OK, the "dmic-wakeup-delay-ms" have been done.
> That said, do we even need a property for this? Does this vary much in
> practice?
>
> If it does, can we not derive this delay from other information (e.g.
> the rates of input clocks and so on)? What exactly determines the
> necessary delay?
Yeah, the DMIC spec indicates wake-up time as 15ms max, so we need to
start recording 15ms after MCLK after testing.
@Woojoo, please correct me if I was wrong.
Thanks.
>
> Thanks,
> Mark.
>
>>
>> sound {
>> diff --git a/sound/soc/rockchip/rk3399_gru_sound.c b/sound/soc/rockchip/rk3399_gru_sound.c
>> index 164b6da..6ab838b 100644
>> --- a/sound/soc/rockchip/rk3399_gru_sound.c
>> +++ b/sound/soc/rockchip/rk3399_gru_sound.c
>> @@ -37,6 +37,8 @@
>>
>> #define SOUND_FS 256
>>
>> +unsigned int rt5514_dmic_delay;
>> +
>> static struct snd_soc_jack rockchip_sound_jack;
>>
>> static const struct snd_soc_dapm_widget rockchip_dapm_widgets[] = {
>> @@ -122,6 +124,9 @@ static int rockchip_sound_rt5514_hw_params(struct snd_pcm_substream *substream,
>> return ret;
>> }
>>
>> + /* Wait for DMIC stable */
>> + msleep(rt5514_dmic_delay);
>> +
>> return 0;
>> }
>>
>> @@ -334,6 +339,15 @@ static int rockchip_sound_probe(struct platform_device *pdev)
>> return -ENODEV;
>> }
>>
>> + /* Set DMIC delay */
>> + ret = device_property_read_u32(&pdev->dev, "dmic-delay",
>> + &rt5514_dmic_delay);
>> + if (ret) {
>> + rt5514_dmic_delay = 0;
>> + dev_dbg(&pdev->dev,
>> + "no optional property 'dmic-delay' found, default: no delay\n");
>> + }
>> +
>> rockchip_dailinks[DAILINK_RT5514_DSP].cpu_name = kstrdup_const(dev_name(dev), GFP_KERNEL);
>> rockchip_dailinks[DAILINK_RT5514_DSP].cpu_dai_name = kstrdup_const(dev_name(dev), GFP_KERNEL);
>> rockchip_dailinks[DAILINK_RT5514_DSP].platform_name = kstrdup_const(dev_name(dev), GFP_KERNEL);
>> --
>> 1.7.9.5
>>
>>
>
>
--
- Xing Zheng
^ permalink raw reply
* [PATCH v2 2/3] musb: sunxi: Remove custom babble handling
From: Bin Liu @ 2016-09-22 13:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160922111901.15337-2-hdegoede@redhat.com>
Hi,
On Thu, Sep 22, 2016 at 02:19:00PM +0300, Hans de Goede wrote:
> The musb-core now a days always treats babble errors in host mode
I don't think this statement is accurate. You might want to change it to
"The musb core already handles babble interrupt" or something else.
Regards,
-Bin.
> as disconnects, so there is no need for the sunxi specific handling
> of this anymore.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch series
> ---
> drivers/usb/musb/sunxi.c | 10 ----------
> 1 file changed, 10 deletions(-)
>
> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
> index 1408245..82eba92 100644
> --- a/drivers/usb/musb/sunxi.c
> +++ b/drivers/usb/musb/sunxi.c
> @@ -186,16 +186,6 @@ static irqreturn_t sunxi_musb_interrupt(int irq, void *__hci)
> if (musb->int_usb)
> writeb(musb->int_usb, musb->mregs + SUNXI_MUSB_INTRUSB);
>
> - /*
> - * sunxi musb often signals babble on low / full speed device
> - * disconnect, without ever raising MUSB_INTR_DISCONNECT, since
> - * normally babble never happens treat it as disconnect.
> - */
> - if ((musb->int_usb & MUSB_INTR_BABBLE) && is_host_active(musb)) {
> - musb->int_usb &= ~MUSB_INTR_BABBLE;
> - musb->int_usb |= MUSB_INTR_DISCONNECT;
> - }
> -
> if ((musb->int_usb & MUSB_INTR_RESET) && !is_host_active(musb)) {
> /* ep0 FADDR must be 0 when (re)entering peripheral mode */
> musb_ep_select(musb->mregs, 0);
> --
> 2.9.3
>
^ permalink raw reply
* [PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion
From: Thomas Gleixner @ 2016-09-22 13:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160922075621.3725-1-jszhang@marvell.com>
On Thu, 22 Sep 2016, Jisheng Zhang wrote:
> Currently ti-32k can be used as a scheduler clock. We properly marked
> omap_32k_read_sched_clock() as notrace but we then call another
> function ti_32k_read_cycles() that _wasn't_ notrace.
>
> Having a traceable function in the sched_clock() path leads to a
> recursion within ftrace and a kernel crash.
Kernel crash? Doesn't ftrace core prevent recursion?
Thanks,
tglx
^ permalink raw reply
* [PATCH v6 1/7] arm/arm64: vgic-new: Implement support for userspace access
From: Vijay Kilari @ 2016-09-22 14:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57E3C9D8.7070906@arm.com>
On Thu, Sep 22, 2016 at 5:38 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 20/09/16 07:12, vijay.kilari at gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>
>> Read and write of some registers like ISPENDR and ICPENDR
>> from userspace requires special handling when compared to
>> guest access for these registers.
>>
>> Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
>> for handling of ISPENDR, ICPENDR registers handling.
>>
>> Add infrastructure to support guest and userspace read
>> and write for the required registers
>> Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>> ---
>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 ----------
>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 98 ++++++++++++++++++++++++++++++++--------
>> virt/kvm/arm/vgic/vgic-mmio.c | 78 ++++++++++++++++++++++++++++----
>> virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
>> 4 files changed, 169 insertions(+), 51 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index b44b359..0b32f40 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -406,31 +406,6 @@ int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
>> return -ENXIO;
>> }
>>
>> -/*
>> - * When userland tries to access the VGIC register handlers, we need to
>> - * create a usable struct vgic_io_device to be passed to the handlers and we
>> - * have to set up a buffer similar to what would have happened if a guest MMIO
>> - * access occurred, including doing endian conversions on BE systems.
>> - */
>> -static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
>> - bool is_write, int offset, u32 *val)
>> -{
>> - unsigned int len = 4;
>> - u8 buf[4];
>> - int ret;
>> -
>> - if (is_write) {
>> - vgic_data_host_to_mmio_bus(buf, len, *val);
>> - ret = kvm_io_gic_ops.write(vcpu, &dev->dev, offset, len, buf);
>> - } else {
>> - ret = kvm_io_gic_ops.read(vcpu, &dev->dev, offset, len, buf);
>> - if (!ret)
>> - *val = vgic_data_mmio_bus_to_host(buf, len);
>> - }
>> -
>> - return ret;
>> -}
>> -
>> int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>> int offset, u32 *val)
>> {
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index 0d3c76a..ce2708d 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -209,6 +209,62 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,
>> return 0;
>> }
>>
>> +static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
>> + gpa_t addr, unsigned int len)
>> +{
>> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>> + u32 value = 0;
>> + int i;
>> +
>> + /*
>> + * A level triggerred interrupt pending state is latched in both
>> + * "soft_pending" and "line_level" variables. Userspace will save
>> + * and restore soft_pending and line_level separately.
>> + * Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
>> + * handling of ISPENDR and ICPENDR.
>> + */
>> + for (i = 0; i < len * 8; i++) {
>> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>> +
>> + if (irq->config == VGIC_CONFIG_LEVEL && irq->soft_pending)
>> + value |= (1U << i);
>> + if (irq->config == VGIC_CONFIG_EDGE && irq->pending)
>> + value |= (1U << i);
>> +
>> + vgic_put_irq(vcpu->kvm, irq);
>> + }
>> +
>> + return value;
>> +}
>> +
>> +static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
>> + gpa_t addr, unsigned int len,
>> + unsigned long val)
>> +{
>> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>> + int i;
>> +
>> + for (i = 0; i < len * 8; i++) {
>> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>> +
>> + spin_lock(&irq->irq_lock);
>> + if (test_bit(i, &val)) {
>> + irq->pending = true;
>> + irq->soft_pending = true;
>> + vgic_queue_irq_unlock(vcpu->kvm, irq);
>> + } else {
>> + irq->soft_pending = false;
>> + if (irq->config == VGIC_CONFIG_EDGE ||
>> + (irq->config == VGIC_CONFIG_LEVEL &&
>> + !irq->line_level))
>> + irq->pending = false;
>> + spin_unlock(&irq->irq_lock);
>> + }
>> +
>> + vgic_put_irq(vcpu->kvm, irq);
>> + }
>> +}
>> +
>> /* We want to avoid outer shareable. */
>> u64 vgic_sanitise_shareability(u64 field)
>> {
>> @@ -358,7 +414,7 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
>> * We take some special care here to fix the calculation of the register
>> * offset.
>> */
>> -#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, bpi, acc) \
>> +#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, ur, uw, bpi, acc) \
>> { \
>> .reg_offset = off, \
>> .bits_per_irq = bpi, \
>> @@ -373,6 +429,8 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
>> .access_flags = acc, \
>> .read = rd, \
>> .write = wr, \
>> + .uaccess_read = ur, \
>> + .uaccess_write = uw, \
>> }
>>
>> static const struct vgic_register_region vgic_v3_dist_registers[] = {
>> @@ -380,40 +438,42 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
>> vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
>> - vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
>> + vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
>> - vgic_mmio_read_enable, vgic_mmio_write_senable, 1,
>> + vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICENABLER,
>> - vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
>> + vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
>> - vgic_mmio_read_pending, vgic_mmio_write_spending, 1,
>> + vgic_mmio_read_pending, vgic_mmio_write_spending,
>> + vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
>> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 1,
>> + vgic_mmio_read_pending, vgic_mmio_write_cpending,
>> + vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
>> - vgic_mmio_read_active, vgic_mmio_write_sactive, 1,
>> + vgic_mmio_read_active, vgic_mmio_write_sactive, NULL, NULL, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
>> - vgic_mmio_read_active, vgic_mmio_write_cactive, 1,
>> + vgic_mmio_read_active, vgic_mmio_write_cactive, NULL, NULL, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
>> - vgic_mmio_read_priority, vgic_mmio_write_priority, 8,
>> - VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>> + vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
>> + 8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ITARGETSR,
>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
>> + vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8,
>> VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICFGR,
>> - vgic_mmio_read_config, vgic_mmio_write_config, 2,
>> + vgic_mmio_read_config, vgic_mmio_write_config, NULL, NULL, 2,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGRPMODR,
>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
>> + vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IROUTER,
>> - vgic_mmio_read_irouter, vgic_mmio_write_irouter, 64,
>> + vgic_mmio_read_irouter, vgic_mmio_write_irouter, NULL, NULL, 64,
>> VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_LENGTH(GICD_IDREGS,
>> vgic_mmio_read_v3_idregs, vgic_mmio_write_wi, 48,
>> @@ -451,11 +511,13 @@ static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
>> REGISTER_DESC_WITH_LENGTH(GICR_ICENABLER0,
>> vgic_mmio_read_enable, vgic_mmio_write_cenable, 4,
>> VGIC_ACCESS_32bit),
>> - REGISTER_DESC_WITH_LENGTH(GICR_ISPENDR0,
>> - vgic_mmio_read_pending, vgic_mmio_write_spending, 4,
>> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ISPENDR0,
>> + vgic_mmio_read_pending, vgic_mmio_write_spending,
>> + vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
>> VGIC_ACCESS_32bit),
>> - REGISTER_DESC_WITH_LENGTH(GICR_ICPENDR0,
>> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 4,
>> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ICPENDR0,
>> + vgic_mmio_read_pending, vgic_mmio_write_cpending,
>> + vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_LENGTH(GICR_ISACTIVER0,
>> vgic_mmio_read_active, vgic_mmio_write_sactive, 4,
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>> index e18b30d..31f85df 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>> @@ -468,6 +468,73 @@ static bool check_region(const struct vgic_register_region *region,
>> return false;
>> }
>>
>> +static const struct vgic_register_region *
>> +vgic_get_mmio_region(struct vgic_io_device *iodev, gpa_t addr, int len)
>> +{
>> + const struct vgic_register_region *region;
>> +
>> + region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
>> + addr - iodev->base_addr);
>> + if (!region || !check_region(region, addr, len))
>> + return NULL;
>> +
>> + return region;
>> +}
>> +
>> +static int vgic_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>> + gpa_t addr, u32 *val)
>> +{
>> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
>> + const struct vgic_register_region *region;
>> + struct kvm_vcpu *r_vcpu;
>> +
>> + region = vgic_get_mmio_region(iodev, addr, sizeof(u32));
>> + if (!region) {
>> + *val = 0;
>> + return 0;
>
> This is not the previous semantic of vgic_uaccess, and I cannot see why
> blindly ignoring an access to an undefined region would be acceptable.
> What am I missing?
AFAIK, the vgic_uaccess is not making any check on undefined region/register.
However, dispatch_mmio_read/write are returning 0 if check of region is failed
>
>> + }
>> +
>> + r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
>> + if (region->uaccess_read)
>> + *val = region->uaccess_read(r_vcpu, addr, sizeof(u32));
>> + else
>> + *val = region->read(r_vcpu, addr, sizeof(u32));
>> +
>> + return 0;
>> +}
>> +
>> +static int vgic_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>> + gpa_t addr, const u32 *val)
>> +{
>> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
>> + const struct vgic_register_region *region;
>> + struct kvm_vcpu *r_vcpu;
>> +
>> + region = vgic_get_mmio_region(iodev, addr, sizeof(u32));
>> + if (!region)
>> + return 0;
>
> Same here.
[...]
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH v2 2/3] musb: sunxi: Remove custom babble handling
From: Hans de Goede @ 2016-09-22 14:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160922135415.GB31827@uda0271908>
Hi,
On 09/22/2016 04:54 PM, Bin Liu wrote:
> Hi,
>
> On Thu, Sep 22, 2016 at 02:19:00PM +0300, Hans de Goede wrote:
>> The musb-core now a days always treats babble errors in host mode
>
> I don't think this statement is accurate. You might want to change it to
> "The musb core already handles babble interrupt" or something else.
It is accurate if you look in the history at drivers/usb/musb
commits around 15-03-10 you will see 2 relevant commits:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/usb/musb?id=b4dc38fd45b63e3da2bc98db5d283a15a637a2fa
"usb: musb: core: simplify musb_recover_work()"
This commits introduces calling musb_root_disconnect(musb)
on babble errors, that was not happening before which is why
I added the custom babble error handling. to the sunxi glue.
And:
"usb: musb: core: always try to recover from babble"
Where the title says it all.
Take these together and I believe that my commit msg:
"The musb-core now a days always treats babble errors in host mode
as disconnects, so there is no need for the sunxi specific handling
of this anymore."
Is quite accurate.
Regards,
Hans
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -This is a new patch in v2 of this patch series
>> ---
>> drivers/usb/musb/sunxi.c | 10 ----------
>> 1 file changed, 10 deletions(-)
>>
>> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
>> index 1408245..82eba92 100644
>> --- a/drivers/usb/musb/sunxi.c
>> +++ b/drivers/usb/musb/sunxi.c
>> @@ -186,16 +186,6 @@ static irqreturn_t sunxi_musb_interrupt(int irq, void *__hci)
>> if (musb->int_usb)
>> writeb(musb->int_usb, musb->mregs + SUNXI_MUSB_INTRUSB);
>>
>> - /*
>> - * sunxi musb often signals babble on low / full speed device
>> - * disconnect, without ever raising MUSB_INTR_DISCONNECT, since
>> - * normally babble never happens treat it as disconnect.
>> - */
>> - if ((musb->int_usb & MUSB_INTR_BABBLE) && is_host_active(musb)) {
>> - musb->int_usb &= ~MUSB_INTR_BABBLE;
>> - musb->int_usb |= MUSB_INTR_DISCONNECT;
>> - }
>> -
>> if ((musb->int_usb & MUSB_INTR_RESET) && !is_host_active(musb)) {
>> /* ep0 FADDR must be 0 when (re)entering peripheral mode */
>> musb_ep_select(musb->mregs, 0);
>> --
>> 2.9.3
>>
^ permalink raw reply
* [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case
From: Christopher Covington @ 2016-09-22 14:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160922094955.GB18637@red-moon>
On 09/22/2016 05:49 AM, Lorenzo Pieralisi wrote:
> On Wed, Sep 21, 2016 at 01:04:57PM -0500, Bjorn Helgaas wrote:
>> On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
>>> On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
>>>> On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
>>>
>>> [...]
>>>
>>>>> None of these platforms can be fixed entirely in software, and given
>>>>> that we will not be adding quirks for new broken hardware, we should
>>>>> ask ourselves whether having two versions of a quirk, i.e., one for
>>>>> broken hardware + currently shipping firmware, and one for the same
>>>>> broken hardware with fixed firmware is really an improvement over what
>>>>> has been proposed here.
>>>>
>>>> We're talking about two completely different types of quirks:
>>>>
>>>> 1) MCFG quirks to use memory-mapped config space that doesn't quite
>>>> conform to the ECAM model in the PCIe spec, and
>>>>
>>>> 2) Some yet-to-be-determined method to describe address space
>>>> consumed by a bridge.
>>>>
>>>> The first two patches of this series are a nice implementation for 1).
>>>> The third patch (ThunderX-specific) is one possibility for 2), but I
>>>> don't like it because there's no way for generic software like the
>>>> ACPI core to discover these resources.
>>>
>>> Ok, so basically this means that to implement (2) we need to assign
>>> some sort of _HID to these quirky PCI bridges (so that we know what
>>> device they represent and we can retrieve their _CRS). I take from
>>> this discussion that the goal is to make sure that all non-config
>>> resources have to be declared through _CRS device objects, which is
>>> fine but that requires a FW update (unless we can fabricate ACPI
>>> devices and corresponding _CRS in the kernel whenever we match a
>>> given MCFG table signature).
>>
>> All resources consumed by ACPI devices should be declared through
>> _CRS. If you want to fabricate ACPI devices or _CRS via kernel
>> quirks, that's fine with me. This could be triggered via MCFG
>> signature, DMI info, host bridge _HID, etc.
>
> I think the PNP quirk approach + PNP0c02 resource put forward by Gab
> is enough.
>
>>> We discussed this already and I think we should make a decision:
>>>
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414722.html
>>>
>>>>>> I'd like to step back and come up with some understanding of how
>>>>>> non-broken firmware *should* deal with this issue. Then, if we *do*
>>>>>> work around this particular broken firmware in the kernel, it would be
>>>>>> nice to do it in a way that fits in with that understanding.
>>>>>>
>>>>>> For example, if a companion ACPI device is the preferred solution, an
>>>>>> ACPI quirk could fabricate a device with the required resources. That
>>>>>> would address the problem closer to the source and make it more likely
>>>>>> that the rest of the system will work correctly: /proc/iomem could
>>>>>> make sense, things that look at _CRS generically would work (e.g,
>>>>>> /sys/, an admittedly hypothetical "lsacpi", etc.)
>>>>>>
>>>>>> Hard-coding stuff in drivers is a point solution that doesn't provide
>>>>>> any guidance for future platforms and makes it likely that the hack
>>>>>> will get copied into even more drivers.
>>>>>>
>>>>>
>>>>> OK, I see. But the guidance for future platforms should be 'do not
>>>>> rely on quirks', and what I am arguing here is that the more we polish
>>>>> up this code and make it clean and reusable, the more likely it is
>>>>> that will end up getting abused by new broken hardware that we set out
>>>>> to reject entirely in the first place.
>>>>>
>>>>> So of course, if the quirk involves claiming resources, let's make
>>>>> sure that this occurs in the cleanest and most compliant way possible.
>>>>> But any factoring/reuse concerns other than for the current crop of
>>>>> broken hardware should be avoided imo.
>>>>
>>>> If future hardware is completely ECAM-compliant and we don't need any
>>>> more MCFG quirks, that would be great.
>>>
>>> Yes.
>>>
>>>> But we'll still need to describe that memory-mapped config space
>>>> somewhere. If that's done with PNP0C02 or similar devices (as is done
>>>> on my x86 laptop), we'd be all set.
>>>
>>> I am not sure I understand what you mean here. Are you referring
>>> to MCFG regions reported as PNP0c02 resources through its _CRS ?
>>
>> Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges
>> reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02
>> device.
>
> Ok, that's agreed. It goes without saying that since you are quoting
> the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device
> _CRS I will consider that a FW bug.
>
>>> IIUC PNP0C02 is a reservation mechanism, but it does not help us
>>> associate its _CRS to a specific PCI host bridge instance, right ?
>>
>> Gab proposed a hierarchy that *would* associate a PNP0C02 device with
>> a PCI bridge:
>>
>> Device (PCI1) {
>> Name (_HID, "HISI0080") // PCI Express Root Bridge
>> Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
>> Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
>> Device (RES0) {
>> Name (_HID, "HISI0081") // HiSi PCIe RC config base address
>> Name (_CID, "PNP0C02") // Motherboard reserved resource
>> Name (_CRS, ResourceTemplate () { ... }
>> }
>> }
>>
>> That's a possibility. The PCI Firmware Spec suggests putting RES0 at
>> the root (under \_SB), but I don't know why.
>>
>> Putting it at the root means we couldn't generically associate it with
>> a bridge, although I could imagine something like this:
>>
>> Device (RES1) {
>> Name (_HID, "HISI0081") // HiSi PCIe RC config base address
>> Name (_CID, "PNP0C02") // Motherboard reserved resource
>> Name (_CRS, ResourceTemplate () { ... }
>> Method (BRDG) { "PCI1" } // hand-wavy ASL
>> }
>> Device (PCI1) {
>> Name (_HID, "HISI0080") // PCI Express Root Bridge
>> Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
>> Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
>> }
>>
>> Where you could search PNP0C02 devices for a cookie that matched the
>> host bridge.o
>
> Ok, I am fine with both and I think we are converging, but the way
> to solve this problem has to be uniform for all ARM partners (and
> not only ARM). Two points here:
>
> 1) Adding a device/subdevice allows people to add a _CRS reporting the
> non-window bridge resources. Fine. It also allows people to chuck in
> there all sorts of _DSD properties to describe their PCI host bridge
> as it is done with DT properties (those _DSD can contain eg clocks
> etc.), this may be tempting (so that they can reuse the same DT
> driver and do not have to update their firmware) but I want to be
> clear here: that must not happen. So, a subdevice with a _CRS to
> report resources, yes, but it will stop there.
> 2) It is unclear to me how to formalize the above. People should not
> write FW by reading the PCI mailing list, so these guidelines have to
> be written, somehow. I do not want to standardize quirks, I want
> to prevent random ACPI table content, which is different.
> Should I report this to the ACPI spec working group ? If we do
> not do that everyone will go solve this problem as they deem fit.
Could you add some checks to fwts?
Cov
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code
Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH] clocksource: timer-digicolor: Unmap region obtained by of_iomap
From: Arvind Yadav @ 2016-09-22 14:21 UTC (permalink / raw)
To: linux-arm-kernel
Free memory mapping, if digicolor_timer_init is not successful.
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
drivers/clocksource/timer-digicolor.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/clocksource/timer-digicolor.c b/drivers/clocksource/timer-digicolor.c
index 10318cc..e6e5e6a 100644
--- a/drivers/clocksource/timer-digicolor.c
+++ b/drivers/clocksource/timer-digicolor.c
@@ -167,12 +167,14 @@ static int __init digicolor_timer_init(struct device_node *node)
irq = irq_of_parse_and_map(node, dc_timer_dev.timer_id);
if (irq <= 0) {
pr_err("Can't parse IRQ");
+ iounmap(dc_timer_dev.base);
return -EINVAL;
}
clk = of_clk_get(node, 0);
if (IS_ERR(clk)) {
pr_err("Can't get timer clock");
+ iounmap(dc_timer_dev.base);
return PTR_ERR(clk);
}
clk_prepare_enable(clk);
@@ -192,6 +194,7 @@ static int __init digicolor_timer_init(struct device_node *node)
&dc_timer_dev.ce);
if (ret) {
pr_warn("request of timer irq %d failed (%d)\n", irq, ret);
+ iounmap(dc_timer_dev.base);
return ret;
}
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox