Re: [Qemu-devel] [PATCH v2 1/4] memory: Avoid to create tiny RAM regions, handled as subpages

[Philippe Mathieu-Daudé]

On 04/05/2018 06:27 AM, Peter Maydell wrote:
> On 5 April 2018 at 02:22, Philippe Mathieu-Daudé <address@hidden> wrote:
>> If an user creates a RAM region smaller than TARGET_PAGE_SIZE,
>> this region will be handled as a subpage.
>> While the subpage behavior can be noticed by an experienced QEMU
>> developper, it might takes hours to a novice to figure it out.
>> To save time to novices, do not allow subpage creation via the
>> memory_region_init_ram_*() functions.
> 
> This commit message doesn't make it clear to me what actually
> goes wrong. Why doesn't the subpage mechanism do the right thing
> here?

Trying to understand a bit more, I think the problem is "you can not
_execute_ from a region smaller than TARGET_PAGE_SIZE", however if this
region is used for I/O this is not a problem (the xilinx-pcie.c case).

In my case I create a 2K SRAM which contains the exception vectors, but
instructions are never fetched because it is handled as I/O.

I suppose my issue is between these two functions:

MemoryRegion *iotlb_to_region(CPUState *cpu, hwaddr index, MemTxAttrs attrs)
{
  int asidx = cpu_asidx_from_attrs(cpu, attrs);
  CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
  AddressSpaceDispatch *d = atomic_rcu_read(&cpuas->memory_dispatch);
  MemoryRegionSection *sections = d->map.sections;

  return sections[index & ~TARGET_PAGE_MASK].mr;
}

tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
{
  int mmu_idx, index, pd;
  void *p;
  MemoryRegion *mr;
  CPUState *cpu = ENV_GET_CPU(env);
  CPUIOTLBEntry *iotlbentry;
  hwaddr physaddr;

  index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
  mmu_idx = cpu_mmu_index(env, true);
  if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
               (addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK)))) {
      if (!VICTIM_TLB_HIT(addr_read, addr)) {
        tlb_fill(ENV_GET_CPU(env), addr, 0, MMU_INST_FETCH, mmu_idx, 0);
      }
  }
  iotlbentry = &env->iotlb[mmu_idx][index];
  pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
  mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
  if (memory_region_is_unassigned(mr)) {
    ...

> 
> Also, a quick grep revealed at least one caller that's currently
> creating a less-than-a-page sized RAM block, in
> hw/pci-host/xilinx-pcie.c. We would need to change all the
> call sites that rely on creating small RAM blocks before we
> could make this throw an error. (Better still would be if we
> could make small lumps of RAM actually work.)