Re: [Xen-devel] [RFC PATCH 21/24] ARM: vITS: handle INVALL command

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 10 Nov 2016, Julien Grall wrote:
> Hi,
> 
> On 10/11/16 00:21, Stefano Stabellini wrote:
> > On Fri, 4 Nov 2016, Andre Przywara wrote:
> > > On 24/10/16 16:32, Vijay Kilari wrote:
> > > > On Wed, Sep 28, 2016 at 11:54 PM, Andre Przywara
> > > > <andre.przywara@xxxxxxx> wrote:
> > > > > The INVALL command instructs an ITS to invalidate the configuration
> > > > > data for all LPIs associated with a given redistributor (read: VCPU).
> > > > > To avoid iterating (and mapping!) all guest tables, we instead go
> > > > > through
> > > > > the host LPI table to find any LPIs targetting this VCPU. We then
> > > > > update
> > > > > the configuration bits for the connected virtual LPIs.
> > > > > 
> > > > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > > > > ---
> > > > >  xen/arch/arm/gic-its.c        | 58
> > > > > +++++++++++++++++++++++++++++++++++++++++++
> > > > >  xen/arch/arm/vgic-its.c       | 30 ++++++++++++++++++++++
> > > > >  xen/include/asm-arm/gic-its.h |  2 ++
> > > > >  3 files changed, 90 insertions(+)
> > > > > 
> > > > > diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c
> > > > > index 6f4329f..5129d6e 100644
> > > > > --- a/xen/arch/arm/gic-its.c
> > > > > +++ b/xen/arch/arm/gic-its.c
> > > > > @@ -228,6 +228,18 @@ static int its_send_cmd_inv(struct host_its *its,
> > > > >      return its_send_command(its, cmd);
> > > > >  }
> > > > > 
> > > > > +static int its_send_cmd_invall(struct host_its *its, int cpu)
> > > > > +{
> > > > > +    uint64_t cmd[4];
> > > > > +
> > > > > +    cmd[0] = GITS_CMD_INVALL;
> > > > > +    cmd[1] = 0x00;
> > > > > +    cmd[2] = cpu & GENMASK(15, 0);
> > > > > +    cmd[3] = 0x00;
> > > > > +
> > > > > +    return its_send_command(its, cmd);
> > > > > +}
> > > > > +
> > > > >  int gicv3_its_map_device(struct host_its *hw_its, struct domain *d,
> > > > >                           int devid, int bits, bool valid)
> > > > >  {
> > > > > @@ -668,6 +680,52 @@ uint32_t gicv3_lpi_lookup_lpi(struct domain *d,
> > > > > uint32_t host_lpi, int *vcpu_id)
> > > > >      return hlpi.virt_lpi;
> > > > >  }
> > > > > 
> > > > > +/* Iterate over all host LPIs, and updating the "enabled" state for a
> > > > > given
> > > > > + * guest redistributor (VCPU) given the respective state in the
> > > > > provided
> > > > > + * proptable. This proptable is indexed by the stored virtual LPI
> > > > > number.
> > > > > + * This is to implement a guest INVALL command.
> > > > > + */
> > > > > +void gicv3_lpi_update_configurations(struct vcpu *v, uint8_t
> > > > > *proptable)
> > > > > +{
> > > > > +    int chunk, i;
> > > > > +    struct host_its *its;
> > > > > +
> > > > > +    for (chunk = 0; chunk < MAX_HOST_LPIS / HOST_LPIS_PER_PAGE;
> > > > > chunk++)
> > > > > +    {
> > > > > +        if ( !lpi_data.host_lpis[chunk] )
> > > > > +            continue;
> > > > > +
> > > > > +        for (i = 0; i < HOST_LPIS_PER_PAGE; i++)
> > > > > +        {
> > > > > +            union host_lpi *hlpip = &lpi_data.host_lpis[chunk][i],
> > > > > hlpi;
> > > > > +            uint32_t hlpi_nr;
> > > > > +
> > > > > +            hlpi.data = hlpip->data;
> > > > > +            if ( !hlpi.virt_lpi )
> > > > > +                continue;
> > > > > +
> > > > > +            if ( hlpi.dom_id != v->domain->domain_id )
> > > > > +                continue;
> > > > > +
> > > > > +            if ( hlpi.vcpu_id != v->vcpu_id )
> > > > > +                continue;
> > > > > +
> > > > > +            hlpi_nr = chunk * HOST_LPIS_PER_PAGE + i;
> > > > > +
> > > > > +            if ( proptable[hlpi.virt_lpi] & LPI_PROP_ENABLED )
> > > > > +                lpi_data.lpi_property[hlpi_nr - 8192] |=
> > > > > LPI_PROP_ENABLED;
> > > > > +            else
> > > > > +                lpi_data.lpi_property[hlpi_nr - 8192] &=
> > > > > ~LPI_PROP_ENABLED;
> > > > > +        }
> > > > > +    }
> > > >         AFAIK, the initial design is to use tasklet to update property
> > > > table as it consumes
> > > > lot of time to update the table.
> > > 
> > > This is a possible, but premature optimization.
> > > Linux (at the moment, at least) only calls INVALL _once_, just after
> > > initialising the collections. And at this point no LPI is mapped, so the
> > > whole routine does basically nothing - and that quite fast.
> > > We can later have any kind of fancy algorithm if there is a need for.
> > 
> > I understand, but as-is it's so expensive that could be a DOS vector.
> > Also other OSes could issue INVALL much more often than Linux.
> > 
> > Considering that we might support device assigment with ITS soon, I
> > think it might be best to parse per-domain virtual tables rather than
> > the full list of physical LPIs, which theoretically could be much
> > larger. Or alternatively we need to think about adding another field to
> > lpi_data, to link together all lpis assigned to the same domain, but
> > that would cost even more memory. Or we could rate-limit the INVALL
> > calls to one every few seconds or something. Or all of the above :-)
> 
> It is not necessary for an ITS implementation to wait until an INVALL/INV
> command is issued to take into account the change of the LPI configuration
> tables (aka property table in this thread).
> 
> So how about trapping the property table? We would still have to go through
> the property table the first time (i.e when writing into the GICR_PROPBASER),
> but INVALL would be a nop.
> 
> The idea would be unmapping the region when GICR_PROPBASER is written. So any
> read/write access would be trapped. For a write access, Xen will update the
> LPIs internal data structures and write the value in the guest page unmapped.
> If we don't want to have an overhead for the read access, we could just
> write-protect the page in stage-2 page table. So only write access would be
> trapped.
> 
> Going further, for the ITS, Xen is using the guest memory to store the ITS
> information. This means Xen has to validate the information at every access.
> So how about restricting the access in stage-2 page table? That would remove
> the overhead of validating data.
> 
> Any thoughts?

It is a promising idea. Let me expand on this.

I agree that on INVALL if we need to do anything, we should go through
the virtual property table rather than the full list of host lpis.

Once we agree on that, the two options we have are:

1) We let the guest write anything to the table, then we do a full
validation of the table on INVALL. We also do a validation of the table
entries used as parameters for any other commands.

2) We map the table read-only, then do a validation of every guest
write. INVALL becomes a NOP and parameters validation for many commands
could be removed or at least reduced.

Conceptually the two options should both lead to exactly the same
result. Therefore I think the decision should be made purely on
performance: which one is faster?  If it is true that INVALL is only
typically called once I suspect that 1) is faster, but I would like to
see some simple benchmarks, such as the time that it takes to configure
the ITS from scratch with the two approaches.


That said, even if 1) turns out to be faster and the approach of choice,
the idea of making the tables read-only in stage-2 could still be useful
to simplify parameters validation and protect Xen from concurrent
changes of the table entries from another guest vcpu. If the tables as
RW, we need to be very careful in Xen and use barriers to avoid
re-reading any guest table entry twice, as the guest could be changing
it in parallel to exploit the hypervisor.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel