Re: [Xen-devel] [RFC PATCH] xen, apic: Setup our own APIC driver and validator for APIC IDs.

[David Vrabel <david.vrabel@xxxxxxxxxx>]

On 22/01/15 15:09, Konrad Rzeszutek Wilk wrote:
> On Thu, Jan 22, 2015 at 10:00:55AM +0000, David Vrabel wrote:
>> On 21/01/15 21:56, Konrad Rzeszutek Wilk wrote:
>>> +static struct apic xen_apic = {
>>> +   .name = "Xen",
>>> +   .probe = probe_xen,
>>> +   /* The rest is copied from the default. */
>>
>> Explicitly initialize all required members here.  memcpy'ing from the
>> default makes it far too unclear which ops this apic driver actually
>> provides.
> 
> That will be hard for two reasons:
>  1) if the 'struct apic' expands and we don't - then we will crash.

This is easy to identify and fix, right?

>  2)  we would need to use the default 'apic' functions ones - which are not
>      necceesarily exposed to the rest of the system. Hence there will
>      be a lot of exposing those.

I think you should try this anyway and then we can evaluate the result.
 I don't think it will be as bad as you fear.

>>>  void __init xen_init_apic(void)
>>>  {
>>>     x86_io_apic_ops.read = xen_io_apic_read;
>>> +
>>> +   memcpy(&xen_apic, apic, sizeof(struct apic));
>>> +   xen_apic.probe = probe_xen;
>>> +   xen_apic.name = "Xen";
>>> +
>>> +   xen_apic.read = xen_apic_read;
>>> +   xen_apic.write = xen_apic_write;
>>> +   xen_apic.icr_read = xen_apic_icr_read;
>>> +   xen_apic.icr_write = xen_apic_icr_write;
>>> +   xen_apic.wait_icr_idle = xen_apic_wait_icr_idle;
>>> +   xen_apic.safe_wait_icr_idle = xen_safe_apic_wait_icr_idle;
>>> +   xen_apic.set_apic_id = xen_set_apic_id;
>>> +   xen_apic.get_apic_id = xen_get_apic_id;
>>> +
>>> +   xen_apic.send_IPI_allbutself = xen_send_IPI_allbutself;
>>> +   xen_apic.send_IPI_mask_allbutself = xen_send_IPI_mask_allbutself;
>>> +   xen_apic.send_IPI_mask = xen_send_IPI_mask;
>>> +   xen_apic.send_IPI_all = xen_send_IPI_all;
>>> +   xen_apic.send_IPI_self = xen_send_IPI_self;
>>> +
>>> +   if (xen_check_x2apic())
>>> +           xen_apic.apic_id_valid = xen_id_always_valid;
>>
>> Just always use xen_id_always_valid regardless of whether the machine
>> has x2apic or not.  It is possible to have more VCPUs that PCPUs.
> 
> In which case perhaps the patch ought to be just simpler and
> instead of having our own 'struct apic' we continue over-writting
> the default one - and just change 'apic_id_valid' to our own.

Please stop pretending that Xen PV guests have a "native" apic with
"specials". We should provide a complete PV-specific apic driver.

David

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