Re: gnumach: [PATCH] - irq as a mach device

[Samuel Thibault]

Hello,

Damien Zammit, le mer. 08 juil. 2020 22:15:23 +1000, a ecrit:
> On 5/7/20 8:13 pm, Samuel Thibault wrote:
> > Yes, but part of that code was not from Linux, but is glue code, I
> > believe this piece is part of it.
> > We can always move code from linux/ anyway, as long as it doesn't pose
> > long-term copyright issues.
> 
> I have a revised patch for the irq handling/sharing.

Thanks! It looks good, just a couple last things:

- you don't need to introduce and call mach_cli(), splhigh already
  disables interrupts, see the implementation in i386/i386/spl.S

- Please keep intr.h's __INTR_H__ guard for consistency.

- When moving code around, you have to copy over the licence statements,
  so here the gpl2+ statement of irq.c
  Ideally we'd dive back to check whether the mask_irq and unmask_irq
  functions could be available under a BSD licence, but gpl2+ is
  compatible with this BSD licence so it's fine to append the gpl2+
  notice.

- For new files, also add licensing terms. Since gnumach is mostly BSD
  we can continue with BSD, see the terms I added to intr.[ch].

> > What we however lack in the current RPC interface is the
> > min/max/alignment of physical address constraints parameters, to make
> > us possibly use VM_PAGE_SEG_DMA32 or even VM_PAGE_SEG_DMA instead of
> > VM_PAGE_SEG_DIRECTMAP, and use the proper power-of-two to respect
> > alignment and release unused pages accordingly.
> 
> I think I have fixed vm_allocate_contiguous, see attached second patch.


> + *     We can specify physical memory range limits and alignment.

pmax is ambiguous, it could be understood as the last possible byte
address, please document that it is the address of the byte just after
the limit, e.g. 0x100000000 for limiting addresses to 32bits.

> -phys_address_t?
[...]
>  routine vm_allocate_contiguous(
>               host_priv       : host_priv_t;
>               target_task     : vm_task_t;
>       out     vaddr           : vm_address_t;
>       out     paddr           : vm_address_t;

For paddr, we really want to use phys_addr_t, otherwise we're stuck in
the 4GiB area.

> +     selector = VM_PAGE_SEL_DMA;
> +#if(VM_PAGE_MAX_SEGS == 4)
> +     if (pmax > VM_PAGE_DMA_LIMIT)
> +             selector = VM_PAGE_SEL_DMA32;
> +     if (pmax > VM_PAGE_DMA32_LIMIT)
> +             selector = VM_PAGE_SEL_DIRECTMAP;
> +     if (pmax > VM_PAGE_DIRECTMAP_LIMIT)
> +             selector = VM_PAGE_SEL_HIGHMEM;
> +#else
> +     if (pmax > VM_PAGE_DMA_LIMIT)
> +             selector = VM_PAGE_SEL_DIRECTMAP;
> +     if (pmax > VM_PAGE_DIRECTMAP_LIMIT)
> +             selector = VM_PAGE_SEL_HIGHMEM;
> +#endif

I don't think you need to distinguish VM_PAGE_MAX_SEGS == 4 from == 3.
Something like this should be fine:

        selector = VM_PAGE_SEL_DMA;
        if (pmax > VM_PAGE_DMA_LIMIT)
#ifdef VM_PAGE_SEL_DMA32
                selector = VM_PAGE_SEL_DMA32;
        if (pmax > VM_PAGE_DMA32_LIMIT)
#endif
                selector = VM_PAGE_SEL_DIRECTMAP;
        if (pmax > VM_PAGE_DIRECTMAP_LIMIT)
                selector = VM_PAGE_SEL_HIGHMEM;

> +     size = vm_page_round(size + palign);

>       alloc_size = (1 << (order + PAGE_SHIFT));
>       npages = vm_page_atop(alloc_size);

I don't think you need to add palign, that could allocate twice as much
as what you need when the size is equel to palign for instance. The
alloc_size will be a power of two already, so you just need to bump
order to vm_page_order(palign) if needed.

> @@ -611,17 +638,21 @@ kern_return_t vm_allocate_contiguous(host_priv, map, 
> result_vaddr, result_paddr,
>               return kr;
>       }
>  
> -     kr = vm_map_pageable(map, vaddr, vaddr + size,
> +     phase = vaddr % palign;
> +     nearest = P2ALIGN(vaddr, palign);

I don't think we need to align the virtual address? It's the CPU which
will access to it through virtual addresses, it doesn't have such
kind of alignment constraints.

> +     *result_vaddr = vaddr + phase;
> +     *result_paddr = pages->phys_addr + phase;

Why adding the phase to the paddr? The palign precisely requests
alignment of the physical pages, we don't want to offset from that.

Samuel