From: Andre Przywara <andre.przywara@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.org,
Julien Grall <julien.grall@arm.com>,
Vijay Kilari <vijay.kilari@gmail.com>,
Shanker Donthineni <shankerd@codeaurora.org>,
Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
Subject: Re: [PATCH v10 25/32] ARM: vITS: handle MAPTI/MAPI command
Date: Mon, 12 Jun 2017 17:10:26 +0100 [thread overview]
Message-ID: <e7657837-438e-483f-f037-4360e444f6c9@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.10.1706091206210.26108@sstabellini-ThinkPad-X260>
Hi,
On 09/06/17 20:14, Stefano Stabellini wrote:
> On Fri, 9 Jun 2017, Andre Przywara wrote:
>> On 02/06/17 18:12, Julien Grall wrote:
>>> Hi Andre,
>>>
>>> On 05/26/2017 06:35 PM, Andre Przywara wrote:
>>>> The MAPTI commands associates a DeviceID/EventID pair with a LPI/CPU
>>>> pair and actually instantiates LPI interrupts. MAPI is just a variant
>>>> of this comment, where the LPI ID is the same as the event ID.
>>>> We connect the already allocated host LPI to this virtual LPI, so that
>>>> any triggering LPI on the host can be quickly forwarded to a guest.
>>>> Beside entering the domain and the virtual LPI number in the respective
>>>> host LPI entry, we also initialize and add the already allocated
>>>> struct pending_irq to our radix tree, so that we can now easily find it
>>>> by its virtual LPI number.
>>>> We also read the property table to update the enabled bit and the
>>>> priority for our new LPI, as we might have missed this during an earlier
>>>> INVALL call (which only checks mapped LPIs). But we make sure that the
>>>> property table is actually valid, as all redistributors might still
>>>> be disabled at this point.
>>>> Since write_itte_locked() now sees its first usage, we change the
>>>> declaration to static.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>> xen/arch/arm/gic-v3-its.c | 27 ++++++++
>>>> xen/arch/arm/vgic-v3-its.c | 138
>>>> ++++++++++++++++++++++++++++++++++++++-
>>>> xen/include/asm-arm/gic_v3_its.h | 3 +
>>>> 3 files changed, 165 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>>>> index 8864e0b..41fff64 100644
>>>> --- a/xen/arch/arm/gic-v3-its.c
>>>> +++ b/xen/arch/arm/gic-v3-its.c
>>>> @@ -876,6 +876,33 @@ int gicv3_remove_guest_event(struct domain *d,
>>>> paddr_t vdoorbell_address,
>>>> return 0;
>>>> }
>>>> +/*
>>>> + * Connects the event ID for an already assigned device to the given
>>>> VCPU/vLPI
>>>> + * pair. The corresponding physical LPI is already mapped on the host
>>>> side
>>>> + * (when assigning the physical device to the guest), so we just
>>>> connect the
>>>> + * target VCPU/vLPI pair to that interrupt to inject it properly if
>>>> it fires.
>>>> + * Returns a pointer to the already allocated struct pending_irq that is
>>>> + * meant to be used by that event.
>>>> + */
>>>> +struct pending_irq *gicv3_assign_guest_event(struct domain *d,
>>>> + paddr_t vdoorbell_address,
>>>> + uint32_t vdevid,
>>>> uint32_t eventid,
>>>> + uint32_t virt_lpi)
>>>> +{
>>>> + struct pending_irq *pirq;
>>>> + uint32_t host_lpi = 0;
>>> This should be INVALID_LPI and not 0.
>>>
>>> [...]
>>>
>>>> +/*
>>>> + * For a given virtual LPI read the enabled bit and priority from the
>>>> virtual
>>>> + * property table and update the virtual IRQ's state in the given
>>>> pending_irq.
>>>> + * Must be called with the respective VGIC VCPU lock held.
>>>> + */
>>>> +static int update_lpi_property(struct domain *d, struct pending_irq *p)
>>>> +{
>>>> + paddr_t addr;
>>>> + uint8_t property;
>>>> + int ret;
>>>> +
>>>> + /*
>>>> + * If no redistributor has its LPIs enabled yet, we can't access the
>>>> + * property table. In this case we just can't update the properties,
>>>> + * but this should not be an error from an ITS point of view.
>>>> + */
>>>> + if ( !read_atomic(&d->arch.vgic.rdists_enabled) )
>>>> + return 0;
>>
>> I was just looking at rdists_enabled, and think that using read_atomic()
>> is a red herring.
>> First rdists_enabled is a bool, so I have a hard time to imagine how it
>> could be read non-atomically.
>
> This is not a good argument, because if we want the read to be atomic,
> then we need to be using one of the _atomic functions regardless of the
> type.
OK, I see your point there. For the records (and to explain my ignorance
;-) I think it applies to a strict C standard point of view only. For
all practical means I think a read into a variable of a native data type
(especially that of a 1-byte sized bool) is always atomic on arm and
arm64 - especially with the load/store architecture of ARM. Also I have
a hard time to imagine intermediate values for a bool ;-), especially
since rdist_enabled only goes from false to true once in a domain's
lifetime.
But nevertheless I can see and agree that to be C standard compliant we
should use read_atomic().
Which makes me wonder if that would be true for other places in the code
as well ...
Another point of confusion may be that read_atomic() on its own does not
seem to be enough here, since we also need the barrier mechanism, maybe
even ACCESS_ONCE. But as I mentioned below this should be covered by the
control flow guarantee is this case.
I wonder if we should brainstorm if the usage of the atomic operations,
the barriers and ACCESS_ONCE is really correct in the current Xen code.
Comparing those to their Linux counterparts at least show some differences.
>> I think the intention of making this read "somewhat special" was to
>> cater for the fact that we write it under the domain lock, but read it
>> here without taking it.
>
> I haven't looked at the specific of rdists_enabled in this
> implementaion, but be aware that in general writing a variable under a
> lock, and reading it atomically is not safe. You either read and write
> under a lock, or read and write atomically.
Agreed. rdists_enabled may be special here because it's a bool and only
goes from false (initialized value) to true once (there is only one
rdists_enabled assignment, which sets it to true).
>> But I think for this case we don't need any
>> special read version, and anyway an *atomic* read would not help here.
>>
>> What we want is to make sure that rdist_propbase is valid before we see
>> rdists_enabled gets true, this is what this check here is for. This
>> should be solved by a write barrier between the two on the other side.
>>
>> Looking at Linux' memory_barriers.txt my understanding is that the
>> matching barrier on the read side does not necessarily need to be an
>> explicit barrier instruction, it could be a control flow dependency as
>> well. And here we have that: we check rdists_enabled and bail out if
>> it's not set, so neither the compiler nor the CPU can reorder this (as
>> this would violate program semantics).
>
> I think that this is true.
>
>
>> Also rdists_enabled is a bit special in that it never gets reset once it
>> became true, very much like the LPI enablement in the GICv3 spec.
>>
>> So I think we can really go with a normal read plus a comment.
>>
>> Does that make sense?
>
> The first motivation isn't right, but I think that the latter
> explanation makes sense.
OK, I think I agree with that.
Cheers,
Andre.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2017-06-12 16:10 UTC|newest]
Thread overview: 96+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-26 17:35 [PATCH v10 00/32] arm64: Dom0 ITS emulation Andre Przywara
2017-05-26 17:35 ` [PATCH v10 01/32] ARM: vGIC: avoid rank lock when reading priority Andre Przywara
2017-05-30 10:47 ` Julien Grall
2017-05-30 21:39 ` Stefano Stabellini
2017-05-31 10:42 ` Julien Grall
2017-06-02 17:44 ` Julien Grall
2017-06-06 17:06 ` Andre Przywara
2017-06-06 17:11 ` Julien Grall
2017-06-06 17:20 ` Andre Przywara
2017-06-06 17:21 ` Julien Grall
2017-06-06 18:39 ` Stefano Stabellini
2017-05-26 17:35 ` [PATCH v10 02/32] ARM: GICv3: setup number of LPI bits for a GICv3 guest Andre Przywara
2017-05-30 10:54 ` Julien Grall
2017-06-06 10:19 ` Andre Przywara
2017-05-26 17:35 ` [PATCH v10 03/32] ARM: vGIC: move irq_to_pending() calls under the VGIC VCPU lock Andre Przywara
2017-05-30 11:08 ` Julien Grall
2017-05-30 21:46 ` Stefano Stabellini
2017-05-31 10:44 ` Julien Grall
2017-06-06 17:24 ` Andre Przywara
2017-06-06 18:46 ` Stefano Stabellini
2017-06-07 10:49 ` Andre Przywara
2017-05-26 17:35 ` [PATCH v10 04/32] ARM: vGIC: rework gic_remove_from_queues() Andre Przywara
2017-05-30 11:15 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 05/32] ARM: vGIC: introduce gic_remove_irq() Andre Przywara
2017-05-30 11:31 ` Julien Grall
2017-06-06 10:19 ` Andre Przywara
2017-05-26 17:35 ` [PATCH v10 06/32] ARM: GIC: Add checks for NULL pointer pending_irq's Andre Przywara
2017-05-30 11:38 ` Julien Grall
2017-06-06 10:19 ` Andre Przywara
2017-06-07 11:19 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 07/32] ARM: GICv3: introduce separate pending_irq structs for LPIs Andre Przywara
2017-05-26 17:35 ` [PATCH v10 08/32] ARM: GIC: export and extend vgic_init_pending_irq() Andre Przywara
2017-05-26 17:35 ` [PATCH v10 09/32] ARM: vGIC: cache virtual LPI priority in struct pending_irq Andre Przywara
2017-05-26 17:35 ` [PATCH v10 10/32] ARM: vGIC: add LPI VCPU ID to " Andre Przywara
2017-05-26 17:35 ` [PATCH v10 11/32] ARM: GICv3: forward pending LPIs to guests Andre Przywara
2017-05-30 11:56 ` Julien Grall
2017-05-30 22:07 ` Stefano Stabellini
2017-05-31 11:09 ` Julien Grall
2017-05-31 17:56 ` Stefano Stabellini
2017-05-31 18:39 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 12/32] ARM: GICv3: enable ITS and LPIs on the host Andre Przywara
2017-05-30 11:58 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 13/32] ARM: vGICv3: handle virtual LPI pending and property tables Andre Przywara
2017-05-26 17:35 ` [PATCH v10 14/32] ARM: introduce vgic_access_guest_memory() Andre Przywara
2017-05-26 17:35 ` [PATCH v10 15/32] ARM: vGICv3: re-use vgic_reg64_check_access Andre Przywara
2017-05-26 17:35 ` [PATCH v10 16/32] ARM: vGIC: advertise LPI support Andre Przywara
2017-05-30 12:59 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 17/32] ARM: vITS: add command handling stub and MMIO emulation Andre Przywara
2017-06-01 18:13 ` Julien Grall
2017-06-08 9:57 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 18/32] ARM: vITS: introduce translation table walks Andre Przywara
2017-06-02 16:25 ` Julien Grall
2017-06-08 9:35 ` Julien Grall
2017-06-08 9:45 ` Andre Przywara
2017-05-26 17:35 ` [PATCH v10 19/32] ARM: vITS: provide access to struct pending_irq Andre Przywara
2017-06-02 16:32 ` Julien Grall
2017-06-02 16:45 ` Julien Grall
2017-06-06 10:19 ` Andre Przywara
2017-06-06 11:13 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 20/32] ARM: vITS: handle INT command Andre Przywara
2017-06-02 16:37 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 21/32] ARM: vITS: handle MAPC command Andre Przywara
2017-05-26 17:35 ` [PATCH v10 22/32] ARM: vITS: handle CLEAR command Andre Przywara
2017-06-02 16:40 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 23/32] ARM: vITS: handle MAPD command Andre Przywara
2017-06-02 16:46 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 24/32] ARM: GICv3: handle unmapped LPIs Andre Przywara
2017-06-02 16:55 ` Julien Grall
2017-06-02 20:45 ` Stefano Stabellini
2017-06-08 9:45 ` Julien Grall
2017-06-08 13:51 ` Andre Przywara
2017-05-26 17:35 ` [PATCH v10 25/32] ARM: vITS: handle MAPTI/MAPI command Andre Przywara
2017-06-02 17:12 ` Julien Grall
2017-06-07 17:49 ` Andre Przywara
2017-06-12 16:33 ` Julien Grall
2017-06-09 11:17 ` Andre Przywara
2017-06-09 19:14 ` Stefano Stabellini
2017-06-12 16:10 ` Andre Przywara [this message]
2017-05-26 17:35 ` [PATCH v10 26/32] ARM: vITS: handle MOVI command Andre Przywara
2017-05-30 22:35 ` Stefano Stabellini
2017-05-31 11:23 ` Julien Grall
2017-05-31 17:53 ` Stefano Stabellini
2017-05-31 18:49 ` Julien Grall
2017-06-02 17:17 ` Julien Grall
2017-06-02 20:36 ` Stefano Stabellini
2017-05-26 17:35 ` [PATCH v10 27/32] ARM: vITS: handle DISCARD command Andre Przywara
2017-06-02 17:21 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 28/32] ARM: vITS: handle INV command Andre Przywara
2017-05-30 22:23 ` Stefano Stabellini
2017-05-26 17:35 ` [PATCH v10 29/32] ARM: vITS: handle INVALL command Andre Przywara
2017-06-02 17:27 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 30/32] ARM: vITS: increase mmio_count for each ITS Andre Przywara
2017-05-26 17:35 ` [PATCH v10 31/32] ARM: vITS: create and initialize virtual ITSes for Dom0 Andre Przywara
2017-06-02 17:31 ` Julien Grall
2017-05-26 17:35 ` [PATCH v10 32/32] ARM: vITS: create ITS subnodes for Dom0 DT Andre Przywara
2017-06-02 17:33 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e7657837-438e-483f-f037-4360e444f6c9@arm.com \
--to=andre.przywara@arm.com \
--cc=Vijaya.Kumar@caviumnetworks.com \
--cc=julien.grall@arm.com \
--cc=shankerd@codeaurora.org \
--cc=sstabellini@kernel.org \
--cc=vijay.kilari@gmail.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).