Re: [PATCH] docs: add PVH specification

["Roger Pau Monné" <roger.pau@citrix.com>]

El 17/09/14 a les 13.59, Jan Beulich ha escrit:
>>>> On 16.09.14 at 17:53, <roger.pau@citrix.com> wrote:
>> +And finally on `EFER` the following features are enabled:
>> +
>> +  * LME (bit 8): Long mode enable.
>> +  * LMA (bit 10): Long mode active.
> 
> Perhaps also worth clarifying which bits are guaranteed to be clear
> (right now one might imply all others, but that's not something we
> can guarantee with forward compatibility in mind). Further I think
> EFLAGS wants mentioning here too, and perhaps the debug registers.

I've added that the SCE and NXE bits will not be enabled, and the
remaining ones will be in an unknown state, possibly we can also add
other bits that will surely not be enabled?

I've also added that RFLAGS is clear when jumping into the kernel entry
point.

>> +
>> +All the segment selectors (`cs`, `ds`, `ss`, `es`, `fs` and `gs`), the
>> +`FS.base` and `GS.base` MSRs are zeroed out.
> 
> For the selector registers, specifying what the hidden portions hold
> is a must I think, at the very least for %cs and %ss.

Done. I've added the following:

The `cs` segment selector is set by Xen with a base of 0x0 and a limit
of 0xfffff. The attributes are set to 0x9b, which describes an
executable and readable code segment only accessible by the most
privileged level.

The remaining segment selectors (`ds`, `ss`, `es`, `fs` and `gs`) are
all set to the same values. Both the selector and the base is set to 0x0
and the limit to 0xfffff. The attributes are set to 0x93, which implies
a read and write data segment only accessible by the most privileged level.

>> MSR registers should be treated
>> +like native.
> 
> Not sure what this is intended to mean.

This was requested in the last review round, but I think that it is
already clear that no hypercalls should be used to write to MSRs, so
I've removed it.

>> +In order to register the callback IDT vector the `HVMOP_set_param` hypercall
>> +is used with the following values:
>> +
>> +    domid = DOMID_SELF
>> +    index = HVM_PARAM_CALLBACK_IRQ
>> +    value = (0x2 << 56) | vector_value
> 
> If we don't have #define-s for these two numbers, we urgently ought
> to add ones.

We already have defines for those two values (in xen.h and hvm/params.h
respectively).

>> +### IO APIC interrupt routing ###
>> +
>> +IO APIC interrupts can be routed over event channels using `PHYSDEVOP`
>> +hypercalls. First the IRQ is registered using the `PHYSDEVOP_map_pirq`
>> +hypercall, as an example IRQ#9 is used here:
>> +
>> +    domid = DOMID_SELF
>> +    type = MAP_PIRQ_TYPE_GSI
>> +    index = 9
>> +    pirq = 9
>> +
>> +After this hypercall, `PHYSDEVOP_alloc_irq_vector` is used to allocate a vector:
>> +
>> +    irq = 9
>> +    vector = 0
>> +
>> +*TODO*: I'm not sure why we need those two hypercalls, and it's usage is not
>> +documented anywhere. Need to clarify what the parameters mean and what effect
>> +they have.
> 
> PHYSDEVOP_alloc_irq_vector has been a dummy for a very long time
> now - nothing should break if this call got omitted.
> 
>> +*NOTE*: when running as Dom0, the guest has to parse the interrupt overwrites
>> +found on the ACPI tables and notify Xen about them.
> 
> ... overrides ...

Removed the mention to PHYSDEVOP_alloc_irq_vector and fixed the spelling
mistake.

Thanks for the review, Roger.