[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 12/22] xen/arm: ITS: Add GICR register emulation

Hi Vijay,

On 01/08/2015 11:25, Vijay Kilari wrote:
I guess you mean vgic_enable_irqs? And no what you've implemented is
definitely not the same as vgic_enable_irqs.

vgic_enable_irqs is locking the pending_irq structure using the vgic
lock of the targeting VCPU (see v_target = ... ->get_target_cpu(...)).

Here, you are locking with the current vCPU, i.e the vCPU which wrote
into the LPI property table.

All the vGIC code is doing the same, so using the wrong locking won't
protect this structure.

    With just vlpi, we cannot get target vcpu without devid. Now
question is there a
need to call gic_raise_guest_irq() for inflight LPIs?

Yes it's necessary. Physical LPIs can come up at any time before the guest enables the virtual LPI. This is because we enable the physical LPIs and route to the guest as soon as the device is assigned to it. You may be interesting by the reading of [1].

You will have to find a way to get the correct vCPU because this may occur more often than you think.

+    id_bits = ((vits->propbase & GICR_PROPBASER_IDBITS_MASK) + 1);
+    if ( id_bits > d->arch.vgic.id_bits )
+        id_bits = d->arch.vgic.id_bits;

As said on v4, you are allowing the possibility to have a smaller
property table than the effective number of LPIs.

An ASSERT in vits_get_priority (patch #16) doesn't ensure the validity
of the size of the property table provided by the guest. This will
surely crash Xen in debug mode, and who knows what will happen in
production mode.

  lpi_size is calculated based on id_bits. If it is smaller, the lpi_size will 
smaller where only size of lpi_size is considered.

Where id_bits is based on what the guest provides in GICR_PROPBASER.
Although the guest, malicious or not, can decide to setup this id_bits
smaller than the number of vLPIs effectively supported by the gic-v3.

In this case, if a vLPI higher than this number is injected you will hit
the ASSERT(vlpi < vits->prop_size) in vits_get_priority. A guest
*should* not be able to crash Xen because it decides to send valid input.

 From 8.11.19, if id_bits is < 8192 (as below statement), GIC treats
LPIs as out of range.

When you quote the spec, please give both the section and which spec. I have about 5 different docs for the GICv3, and I had to guess which one you were using.

"If the value of this field is less than 0b1101, indicating that the
largest interrupt ID is less than 8192
(the smallest LPI interrupt ID), the GIC will behave as if all
physical LPIs are out of range."

Thank you for the quoting. I helps me to not a latent bug in your LPI
property table emulation. I should have read more carefully the spec.

The field IDBits gives you the number of interrupt ID bits supported. The LPI property table size is only describing the LPI, i.e the offset 0 of the table is the IntID 8192.

So when you compute the size of the table, you have to substract 8192. Otherwise you will remove 2 4KB pages from the guest which could be used by it. You will also, possible Xen unsafe as we may read out of the pending IRQ array (we are trusting the handler to be registered on valid input).

Based on this, we should make a check on this GICR_PROPBASER.id_bits
before injecting LPI to domain  when LPI is received.

And doing what? The paragraph you quote doesn't say anything on what happen when the LPI interrupt ID is higher than the number of bits. It only explain what happen the this number if higher and smaller than a bound.

What would happen if the LPI is injected before the GICR_PROPBASER is enabled? See for more details on the problem here [1]

I think, you just have to make sure that the function reading the priority (i.e vits_get_priority) is not trying to read out of the array and return a dummy value (??).

I added it because, after updating my Linux kernel driver, I see that it
  is making 32-bit access to TYPER register

This change should go in a separate patch then. It's not related to this

   Why separate patch?.  This change could be part of GICR* reg emulation
done for LPI

Because this change is not part of the re-distributor emulation done for LPI. You don't even mention it in your commit message. I discovered it by comparing on what you did with the previous version.

Furthermore, as said earlier, if you handle 32-bit access for GICR_TYPER you have to do it for every others registers.

Anyway, I will send a patch myself to handle 32-bit access on 64-bit registers.


[1] http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg02591.html

Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.